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

[fix] allow any top-level keys in svelte config #2267

Merged
merged 4 commits into from Aug 23, 2021
Merged

[fix] allow any top-level keys in svelte config #2267

merged 4 commits into from Aug 23, 2021

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 22, 2021

Fixes #2253

Allow any top-level keys to be added to the svelte config, essentially skipping validation for the top-level keys only.

Unrelated side note: I'm not sure why we're not using something like superstruct to handle validation. And if the spec of the svelte config should live here?

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 and pnpm check

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. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2021

🦋 Changeset detected

Latest commit: 66fdaa8

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

Copy link
Member

@ignatiusmb ignatiusmb left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of questions

packages/kit/src/core/config/index.js Outdated Show resolved Hide resolved
packages/kit/types/config.d.ts Outdated Show resolved Hide resolved
@bluwy bluwy requested a review from ignatiusmb August 22, 2021 16:45
@benmccann
Copy link
Member

Unrelated side note: I'm not sure why we're not using something like superstruct to handle validation. And if the spec of the svelte config should live here?

I'd be happy to use superstruct or something similar. I hadn't seen that library and think we only coded it ourselves out of unfamiliarity with other options. I've heard other also ponder if we should have a package specifically for svelte config, but no one's really made a proposal or wanted to take on that work. I think it's something folks would be open to though since I have heard it before

packages/kit/src/core/config/index.spec.js Outdated Show resolved Hide resolved
@ignatiusmb
Copy link
Member

I think misplacing kit options outside config.kit is pretty common and the hint might still be useful to have around for some people, perhaps we can still iterate over it even if keypath is config, but have a couple of ifs for it inside to skip them if it's not in config.kit.

@bluwy
Copy link
Member Author

bluwy commented Aug 23, 2021

superstruct

Yeah, it would be nice to use that instead to also remove the need to maintain its tests. But I guess there's already an infrastructure setup for custom validation so until some issue comes up with it then we could consider superstruct. I'd be happy to tackle that.

separate package for svelte config

It would be nice to have something like postcss-load-config but for svelte so it can also be used by vite-plugin-svelte and the language tools. I'm also not sure if vite-plugin-svelte should be polluting the top-level keys in svelte config either since there really isn't a clear spec for that (?). But it's a bit off-topic now :)

warn misplaced kit options

@ignatiusmb I'd be afraid that it would cause false positives for that. If one day a package decides to use a top-level key that's the same as config.kit.*, then it wouldn't work in kit even though it should.

@ignatiusmb
Copy link
Member

I'd say the odds are pretty slim and we should not encourage packages, at least community-made, to directly use top-level config and instead put them under a name like kit does. I'm thinking we could at least warn them in the console or do a double-check using its value type or if the keys matches what we expected in the config. But, I guess we can discuss more when this is handled outside of kit, so this should do for now.

@ignatiusmb ignatiusmb merged commit 8fc5e52 into sveltejs:master Aug 23, 2021
@ignatiusmb
Copy link
Member

Thanks!

@bluwy bluwy deleted the gh-2253 branch August 23, 2021 03:45
@benmccann
Copy link
Member

Sure, feel free to send a PR for superstruct if you're interested

I agree that options should be under a namespace like kit

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.

Ability to disable warnings like css-unused-selector
3 participants