rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

feat: svelte config util

Open dominikg opened this issue 3 years ago • 22 comments

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

dominikg avatar Sep 29 '21 12:09 dominikg

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?

dummdidumm avatar Sep 29 '21 14:09 dummdidumm

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

dominikg avatar Sep 29 '21 14:09 dominikg

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

dummdidumm avatar Sep 29 '21 15:09 dummdidumm

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.

bluwy avatar Sep 29 '21 15:09 bluwy

Linking to past discussion about this for reference: https://github.com/sveltejs/vite-plugin-svelte/issues/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

benmccann avatar Sep 29 '21 23:09 benmccann

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.

dummdidumm avatar Oct 09 '21 20:10 dummdidumm

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.

bluwy avatar Oct 10 '21 04:10 bluwy

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 avatar Oct 10 '21 10:10 dominikg

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 ( https://github.com/sveltejs/kit/pull/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 avatar Oct 13 '21 09:10 dominikg

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?

dominikg avatar Oct 13 '21 12:10 dominikg

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 🙏

kaisermann avatar Oct 13 '21 12:10 kaisermann

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.

bluwy avatar Oct 13 '21 14:10 bluwy

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

dominikg avatar Oct 13 '21 14:10 dominikg

@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?

dummdidumm avatar Oct 13 '21 14:10 dummdidumm

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.

DeMoorJasper avatar Oct 13 '21 16:10 DeMoorJasper

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

dummdidumm avatar Oct 13 '21 18:10 dummdidumm

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.

bluwy avatar Oct 14 '21 14:10 bluwy

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

benmccann avatar Oct 22 '21 21:10 benmccann

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

non25 avatar Oct 22 '21 21:10 non25

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.

dummdidumm avatar Oct 23 '21 06:10 dummdidumm

leaving this here as a reminder: https://github.com/sveltejs/kit/pull/3219

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

dominikg avatar Jan 06 '22 16:01 dominikg