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: use new server.restart api and update vite peer to ^2.7.0 #223

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

dominikg
Copy link
Member

@dominikg dominikg commented Dec 8, 2021

instead of fake change event on config file

@dominikg
Copy link
Member Author

dominikg commented Dec 8, 2021

unfortunately this doesn't work for sveltekit out of the box (see packages/e2e-test/kit-node )
On restart, index.html is not found/loaded in the browser. needs investigation

@dominikg dominikg changed the title refactor: update peerDependency on vite to ^2.7.0 refactor: use new server.restart api and update peerDependency on vite to ^2.7.0 Dec 9, 2021
@dominikg dominikg changed the title refactor: use new server.restart api and update peerDependency on vite to ^2.7.0 refactor: use new server.restart api and update vite peer to ^2.7.0 Dec 9, 2021
@dominikg
Copy link
Member Author

dominikg commented Dec 9, 2021

The reason it doesn't work for kit is that kit does extra work when it initializes the devserver instance the first time which is not repeated when we call server.restart()

Pointers:
https://github.com/sveltejs/kit/blob/ae7f7904c4f3c111dbff1ad1209ad92fb6f057cf/packages/kit/src/cli.js#L94
https://github.com/sveltejs/kit/blob/master/packages/kit/src/core/dev/index.js

Esp. removing middlewares would have to be done again
https://github.com/sveltejs/kit/blob/ae7f7904c4f3c111dbff1ad1209ad92fb6f057cf/packages/kit/src/core/dev/index.js#L555
related: vitejs/vite#4640 and vitejs/vite#5720

this could be achieved by using a special "vite-plugin-kit" that implements configResolved, configureServer and buildStart hooks to update vite instead of doing it once.

If this is wanted for kit, it should be implemented in a PR in kit repo, once that has been merged we can remove the "not for kit" constraint in here.

@dominikg dominikg requested a review from bluwy December 9, 2021 20:39
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM. Re sveltekit, looks like the code is a fine compromise for now until we find a cleaner way.

@dominikg
Copy link
Member Author

don't merge/release until kit repo has been updated to vite 2.7

@bluwy bluwy merged commit 4021352 into main Dec 14, 2021
@bluwy bluwy deleted the refactor/restart-on-config-change branch December 14, 2021 17:00
@github-actions github-actions bot mentioned this pull request Dec 15, 2021
@github-actions github-actions bot mentioned this pull request Jul 13, 2022
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.

3 participants