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

[chore] provide Vite config via plugin #5073

Merged
merged 5 commits into from May 27, 2022
Merged

[chore] provide Vite config via plugin #5073

merged 5 commits into from May 27, 2022

Conversation

benmccann
Copy link
Member

@benmccann benmccann commented May 25, 2022

Kit is now simply a Vite plugin in dev mode

We could move it to a new location and export it as @sveltejs/kit/vite, but I didn't want to move files around in the same PR where I'm refactoring things to make it more manageable to understand what's changing. I'll do that after this PR is merged in a follow up

@changeset-bot
Copy link

changeset-bot bot commented May 25, 2022

🦋 Changeset detected

Latest commit: f1e52dc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines 71 to 74
const config = {
plugins: [...(vite_config.plugins || []), svelte(svelte_config), sveltekit(svelte_config)]
};
config.server = config.server || {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const config = {
plugins: [...(vite_config.plugins || []), svelte(svelte_config), sveltekit(svelte_config)]
};
config.server = config.server || {};
const config = {
plugins: [...(vite_config.plugins || []), svelte(svelte_config), sveltekit(svelte_config)],
server: {}
};

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a more natural way to write it, but upsets TypeScript. TypeScript see the type annotation which says server can be optional and that takes precedence. If we want to do it the suggested way then I also need to add & {server: import('vite').ServerOptions} to the type, which seemed even uglier to me. I just noticed the way I have it written currently can be simplified though

@benmccann
Copy link
Member Author

I'm going to merge this since it seems like we're pretty much agreed on the details at this point that way I can get started on the next PR. Feel free to leave comments still on either this PR or the next one and I can tweak any remaining items still

@benmccann benmccann merged commit 712b4d8 into master May 27, 2022
@benmccann benmccann deleted the config-loading branch May 27, 2022 02:05
@seandlg
Copy link

seandlg commented Jun 21, 2022

This likely caused a weird ParseError: The keyword 'let' is reserved error for me. Removing a dangling vite.config.js made things go back to normal. See this discord discussion for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants