Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

fix: updated Svelte and SvelteKit Framework Info to latest #846

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brittneypostma
Copy link

@brittneypostma brittneypostma commented Nov 28, 2022

πŸŽ‰ 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 and vite dev commands and directories


For us to review and ship your PR efficiently, please perform the following steps:

  • [ x] Open a bug/issue before writing your code πŸ§‘β€πŸ’». This
    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.
  • [ x] Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • [ x] Make sure the status checks below are successful βœ…

A picture of a cute animal (not mandatory, but encouraged)
fancy capybara with a wine glass

…te build and dev commands and directories
@netlify
Copy link

netlify bot commented Nov 28, 2022

βœ… Deploy Preview for framework-info ready!

Name Link
πŸ”¨ Latest commit b93006e
πŸ” Latest deploy log https://app.netlify.com/sites/framework-info/deploys/6384c0fae15cc20008bc2544
😎 Deploy Preview https://deploy-preview-846--framework-info.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@conventional-commit-lint-gcf
Copy link

πŸ€– 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 automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@brittneypostma
Copy link
Author

I need to update the readme and run the release command before this is ready.

@brittneypostma brittneypostma changed the title Updated Svelte and SvelteKit Framework Info to latest Fix: Updated Svelte and SvelteKit Framework Info to latest Nov 28, 2022
@github-actions github-actions bot added the type: bug code to address defects in shipped code label Nov 28, 2022
@ascorbic ascorbic changed the title Fix: Updated Svelte and SvelteKit Framework Info to latest fix: Updated Svelte and SvelteKit Framework Info to latest Nov 28, 2022
@ascorbic ascorbic changed the title fix: Updated Svelte and SvelteKit Framework Info to latest fix: updated Svelte and SvelteKit Framework Info to latest Nov 28, 2022
Comment on lines -11 to +12
"command": "npm run dev",
"port": 5000,
"command": "vite dev",
"port": 5173,
Copy link

@rendall rendall Nov 30, 2022

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?

Copy link
Author

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.

Copy link
Member

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.

Copy link

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!

πŸ‘πŸ»

@brittneypostma brittneypostma enabled auto-merge (squash) November 30, 2022 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte should not assume dev server is available
3 participants