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

feat: svelte config util #59

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

Conversation

dominikg
Copy link
Member

Please share additional ideas. I'm willing to implement it once finalized.

@dummdidumm
Copy link
Member

I like this very much! About drawbacks: None of the listed are of concern to me, I think they force us to have a good talk about the config which is a good thing. I also don't think that the overhead is that big (bundlesize-wise).

I have one question about namespaces: I remember you mentioning that you could repeat top level entries in the namespace to overwrite the top level entries. Is that part of the solution? If yes, could you detail that part a little?

@dominikg
Copy link
Member Author

I had that added already but changed my mind. I think the inlineOptions in loadConfig give tools enough freedom

@dummdidumm
Copy link
Member

Sounds good. If we later feel it's needed we can always add it in a backwards-compatible way

@bluwy
Copy link
Member

bluwy commented Sep 29, 2021

Regarding validation, I think it's of Rich's interest to re-use the code from Kit (also mentioned in RFC), and avoid a third-party library for it. I've made a pull request in the past to convert to superstruct, but was closed because the package size is quite hefty, and the error messages aren't really friendly.

In my experience refactoring that, making Kit's validation generic won't be easy unless we draw the line on the kind of validations we would only support. Otherwise, if someone can suggest an alternative mini validation library, would be nice too.

Or another route we could go is to dismiss validation all together and rely on types only.

@benmccann
Copy link
Member

Linking to past discussion about this for reference: sveltejs/vite-plugin-svelte#146. There was a fair amount of discussion around what should be a top-level shared key vs living under a namespace for each bundler

@dummdidumm
Copy link
Member

One more question about transpiling TS->JS: would that affect the config file only, or would all transitive imports be transpiled as well? People will expect the latter I guess, but I'm not sure how this could be implemented right now.

@bluwy
Copy link
Member

bluwy commented Oct 10, 2021

would that affect the config file only, or would all transitive imports be transpiled as well?

For esbuild, since it's a bundler we can bundle the config file and run it, similar to how Vite works. I'm not sure about typescript though, looks like Jest uses ts-node to load it. If we go down that path, we could as well rely on https://github.com/lukeed/tsm instead. Edit: Actually, tsm uses esbuild too.

@dominikg
Copy link
Member Author

postcss-load-config uses ts-node too
https://github.com/postcss/postcss-load-config/blob/64528c6be7ced63a77574d2092dd0db2c4320367/src/index.js#L104

but i'd rather not introduce that and stick with esbuild as thats more likely to be available already via vite/sveltekit.

@dominikg
Copy link
Member Author

Had a chat with @bluwy about this, takeaways:

  • types for common namespaces like preprocess / compilerOptions etc can be provided by @sveltejs/config but in general tools are responsible for their own types
  • loading .ts files can be done similar to vite by lazy requiring esbuild, bundle .ts to .js on the fly and then using common .js loading
    • async config feature allows users to plug in any other loading mechanism they like, so if esbuild doesn't tick their box, that would be an escape hatch
  • validation rules are complex to define a common ground for, json schema is ugly, superstruct is a bigger dependency and was discarded in kit before ( [chore] use superstruct for config validation kit#2366 ) so to keep the scope of this RFC and the initial release of @sveltejs/config more manageable, it should be dropped and tools keep their own validations until a later feature release adds validation support

@dominikg
Copy link
Member Author

It seems like people are generally in favor of this, so what needs to be done to finalize this RFC and move it forward?
Most common tooling maintainers already chimed in, maybe @kaisermann has something to add?

Is there an "acting maintainer" for svelte-loader that should be pinged about this?

@kaisermann
Copy link
Member

I've just read the RFC and it looks good to me! Got nothing to add and I agree with leaving the validation for a later release. Very nice standardization initiative 🙏

@bluwy
Copy link
Member

bluwy commented Oct 13, 2021

Maybe @DeMoorJasper from parcel-plugin-svelte could chime in too. IIRC parcel has its own config reading logic, plus parcel will have some big news coming soon (might affect this). So if it's not possible for them to use @sveltejs/config, perhaps it can be based on as a spec too.

@dominikg
Copy link
Member Author

cc @drwpow and @FredKSchott for snowpack plugin svelte
cc @rixo for svelte-hmr (and svench?)

@dummdidumm
Copy link
Member

@Conduitry might have some insights for rollup-plugin-svelte about this. @lukeed might have some ideas about transpiling TS on the fly, maybe the new tsm-package can help with that?

@DeMoorJasper
Copy link

For parcel we'd need some way to invalidate our cache on config changes, so in this case we'd need to know which files have been looked at to find the config and which file(s) contain the config.

Also preferably a preference towards non-js config files as js configs can't be statically checked and would require us to invalidate the cache of every svelte file every time the bundler restarts.

A way to define some defaults would probably also be nice.

@dummdidumm
Copy link
Member

Non-js config files will very likely be impossible, kit already has some settings which expect a function

@bluwy
Copy link
Member

bluwy commented Oct 14, 2021

For parcel we'd need some way to invalidate our cache on config changes, so in this case we'd need to know which files have been looked at to find the config and which file(s) contain the config.

This might be tricky since ideally we would normally import() the file, and node doesn't give much info pass that. For vite-plugin-svelte, we had Vite's file watcher watching svelte.config.js and it would reload the dev server whenever the config changes. Re "know which files have been looked at to find the config", the loadConfig() function would work naively by searching directly in ${fromDir}/svelte.config.js.

A way to define some defaults would probably also be nice.

I think inlineConfig would cover that.

@benmccann
Copy link
Member

Is there an "acting maintainer" for svelte-loader that should be pinged about this?

@non25 has probably been the most active person on that repo

@non25
Copy link

non25 commented Oct 22, 2021

Looks good, maybe we could finally configure compiler-related options for language server, eslint and plugin/loader in one place.

@dummdidumm
Copy link
Member

We need to leave things like prettier or eslint out of this since they have an established config style already, and in the case of eslint they need commonjs configs. But I agree that rollup/webpack plugins could start using this, too, and as Dominik mentioned in the RFC, provide their own options within a namespace like rollupPlugin.

@dominikg
Copy link
Member Author

dominikg commented Jan 6, 2022

leaving this here as a reminder: sveltejs/kit#3219

add error handling for missing config file including links to documentation how to add/use svelte config

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.

None yet

7 participants