Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: top-level createEnvironment is client-only #18475

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

patak-dev
Copy link
Member

Description

@bluwy proposed to remove the top level createEnvironment as it didn't make sense as a default. The types would get more complex though, and after #18465, we can instead make the top-level createEnvironment be client-only instead. This is also interesting because now a single environment could potentially be configured as:

export default defineConfig(electronEnvironment({
  build: {
    outDir: '/app',
  }
}))

Maybe we should recommend the form below, but I think the above is interesting to consider too (it isn't the reason for this PR though).

export default defineConfig({
  build: {
    outDir: '/app',
  },
  environments: {
    client: electronEnvironment()
  }
})

@patak-dev patak-dev requested a review from bluwy October 25, 2024 21:08
@patak-dev patak-dev added the feat: environment API Vite Environment API label Oct 25, 2024
@patak-dev patak-dev added this to the 6.0 milestone Oct 25, 2024
sapphi-red
sapphi-red previously approved these changes Oct 28, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it back and forth, I think this fits more to the current way of configuring Vite.

@sapphi-red sapphi-red self-requested a review October 28, 2024 06:46
@sapphi-red sapphi-red dismissed their stale review October 28, 2024 06:46

I approved the wrong PR 😅

sapphi-red
sapphi-red previously approved these changes Oct 28, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

LGTM too. I think I prefer still the form of environments.client if you have to configure createEnvironment for the client, but it's something we can figure out again later.

@patak-dev patak-dev enabled auto-merge (squash) October 28, 2024 09:30
@patak-dev patak-dev disabled auto-merge October 28, 2024 09:31
@patak-dev patak-dev merged commit 6022fc2 into main Oct 28, 2024
11 of 12 checks passed
@patak-dev patak-dev deleted the refactor/top-level-create-environment-is-client-only branch October 28, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: environment API Vite Environment API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants