-
Notifications
You must be signed in to change notification settings - Fork 21
fix: updated Svelte and SvelteKit Framework Info to latest #846
base: main
Are you sure you want to change the base?
Conversation
…te build and dev commands and directories
✅ Deploy Preview for framework-info ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
I need to update the readme and run the release command before this is ready. |
"command": "npm run dev", | ||
"port": 5000, | ||
"command": "vite dev", | ||
"port": 5173, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong here and so this is a "single comment" and not a "review". Please forgive my exceeding ignorance here, especially if none of this is helpful.
I don't know that this will actually fix #844. The assumption that causes #844 seems to be that a [svelte] dependency implies that a framework server is running (perhaps via svelte-kit
).
{
"detect": {
"npmDependencies": ["svelte"]
},
"dev": {
"command": "vite dev",
"port": 5173
}
}
In any case, this PR assumes that vite
is the task runner which is probably not right. There might be a problem with assuming that a framework-based dev server is running at all. Maybe a fallback to a static server?
Edit: I struck out a number of my own incorrect assumptions, below. svelte-kit
does indeed require vite
and there already is a svelte-kit.json file.
This PR doesn't seem to address that assumption, so much as assume that since svelte-kit
is being used, so then vite
must also be (I have not used svelte-kit, but I suspect that other bundlers / task runners are possible?)
Could it be confusing that this filename is svelte.json
? Would it make more sense to name it svelte-kit.json
since the assumption here seems to be svelte-kit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SvelteKit does require vite and uses it for the dev and build commands. Svelte (vanilla) recommends the usage of the Vite template which also uses Vite for the dev and build commands. I think this is the best option going forward to detect the framework correctly and if there is a custom setup, it will need to be manually configured in the netlify.toml
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these values are meant to be the defaults for a framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. All is clear, then!
👍🏻
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #844
Should also fix issue #5280 on the CLI
edited svelte and sveltekit framework info to update to the latest
vite build
andvite dev
commands and directoriesFor us to review and ship your PR efficiently, please perform the following steps:
ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a
typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)