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

simplify create-svelte #989

Merged
merged 11 commits into from
Apr 13, 2021
Merged

simplify create-svelte #989

merged 11 commits into from
Apr 13, 2021

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Apr 12, 2021

#987

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@Rich-Harris Rich-Harris marked this pull request as ready for review April 13, 2021 05:00
@Rich-Harris
Copy link
Member Author

I think this is a bit simpler — no CSS/SASS/LESS option, and the template (soon to be templates) is authored in TS and converted to JS at build time.

Additionally, the modifiers no longer make brittle edits via code; instead we have files inside directories like

shared/+eslint-prettier

which means 'copy this if the user chose eslint but not prettier, regardless of their typescript choice'. It results in some duplication, but I think it'll prove easier to grok.

Also, since more of the work is happening at build time, the create-svelte package gets even tinier.

Not sure what's going on with the lint failure, will investigation in the morning.

@benmccann
Copy link
Member

benmccann commented Apr 13, 2021

FYI, lint is failing

The naming scheme seems like it could create an exponential explosion. E.g. we have four eslint configs right now. If we add a prompt for legacy mode (i.e. IE support) that used @babel/eslint-plugin I think we would need to create a +babel and -babel version (or +legacy/-legacy if we want to call it that instead) of each of the existing files so we'd have eight of them. We might be even more likely to hit this for the package.json since it seems likely that any new option might add a dependency

@dummdidumm dummdidumm mentioned this pull request Apr 13, 2021
1 task
Rich Harris and others added 3 commits April 13, 2021 09:41
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@Rich-Harris
Copy link
Member Author

I hear the point re combinatorial explosion, but my sense is that it's less of an issue than it appears. Firstly, the package.json files are merged (i.e. the eslint and eslint-plugin-svelte3 dev dependencies are only declared in the +eslint directory, not +eslint and +eslint-prettier and +eslint-typescript and +eslint+prettier and +eslint+prettier-typescript and +eslint+prettier+typescript and +eslint+typescript).

I can't imagine us wanting to add a Babel option other than to support a legacy mode, in which case we wouldn't need @babel/eslint-plugin IIUC — that's only to extend the ESLint parser with support for new language features. To my mind that falls very much under 'if you want to add a bunch of complexity to your project, go nuts, but you're on your own'.

In cases where tools don't overlap (i.e. adding Babel without adding the ESLint plugin), no combinatorial explosion will arise; it's just like that right now because TypeScript and Prettier and ESLint do overlap. It certainly could become an issue in future but I think we can probably cross that bridge when we come to it. I still think on balance it's better to have things be declarative even at the cost of some duplication. It's definitely not clear-cut though.

@dummdidumm
Copy link
Member

Yeah I'd also say if we add a bunch more stuff and there are too many combinations, we can always revisit this decision. So I'm good with these changes.

@Rich-Harris Rich-Harris merged commit 9bb747f into master Apr 13, 2021
@Rich-Harris Rich-Harris deleted the gh-987 branch April 13, 2021 14:21

// Consult https://github.com/sveltejs/svelte-preprocess
// for more information about preprocessors
preprocess: sveltePreprocess(),
Copy link
Member

@ignatiusmb ignatiusmb Apr 13, 2021

Choose a reason for hiding this comment

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

Shouldn't this be at the same level as kit?

Edit: A couple minutes too late :(

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, thanks — fixed in 3c41d07

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.

4 participants