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

Better documentation for config options #146

Closed
dominikg opened this issue Aug 20, 2021 · 11 comments · Fixed by #151
Closed

Better documentation for config options #146

dominikg opened this issue Aug 20, 2021 · 11 comments · Fixed by #151
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@dominikg
Copy link
Member

Describe the problem

Currently it only links to options.ts for details and while there are jsdoc comments on the interface, this is not really discoverable, leading to users not learning about them when they might need em.

Describe the proposed solution

Proper documentation for all options, with examples/explaination of how they work in practice.

Alternatives considered

keep status quo

Importance

would make my life easier

@dominikg dominikg added the enhancement New feature or request label Aug 20, 2021
@dominikg dominikg added this to the 1.0.0 milestone Aug 20, 2021
@dominikg
Copy link
Member Author

for example this kit issue sveltejs/kit#2253 where onwarn wasn't known

@benmccann
Copy link
Member

@bluwy said that right now the options are in the global namespace (sveltejs/kit#2267 (comment)). I think options should probably be under a key like vitePlugin similar to how we have kit unless they're options that are likely to be shared across plugins (like possibly 'onwarn')

@dominikg
Copy link
Member Author

agreed, include, exclude, extensions, emitCss, compilerOptions, preprocess are the ones that could be shared imho. maybe hot too, although that would better be renamed to hmr and i don't know which other tools currently wrap svelte-hmr in a way that would allow this to be shared.

@bluwy
Copy link
Member

bluwy commented Aug 23, 2021

Yeah putting stuff in vitePlugin would be the best. And re @dominikg's list for default top-level fields, those seem reasonable to me, but it also feels a bit rollup-centric, and might not be transferable to the likes of @snowpack/plugin-svelte, webpack-loader and parcel-plugin-svelte.

I think before we start making a breaking change to move these options to vitePlugin, we may want a proper spec to outline which fields are possible at the top-level, and which should be namespaced. Or maybe even a dedicated config loader to enforce this. With that other tools could easily adopt the config more without they're own interpretation.

PS: IIRC Parcel has its own config loading system (for auto server reload), might need to take into account of that.

@benmccann
Copy link
Member

Yeah, I was wondering if they should be under a rollup key or similar or if they should be standardized. Another option if they were going to be shared would be to have them go under a generic key like build

@snowpack/plugin-svelte seems to look for them currently in the top-level as does parcel-plugin-svelte

For webpack, extensions gets specified in the webpack.config.js. Snowpack calls it input

Vite has hot, webpack has hotReload and hotOptions, Snowpack has hmrOptions, Rollup and Parcel don't support hot reload.

All the bundler plugins seem to use compilerOptions and preprocess. There's no reason to have the hot options called three different things

However, some of the keys are specific to a particular tool. E.g. include and exclude are Rollup-specific. I'm not sure if it makes sense to split the options between two different places like common options one place and Rollup options another place. Probably each plugin should have its own key where it reads everything from. But we could suggest to the plugin authors that they standardize the names a bit (e.g. rename Snowpack's input to extensions and standardize the key for hot options) even if it's not enforced

@bluwy
Copy link
Member

bluwy commented Aug 23, 2021

A generic key like build sounds nice. I was initially thinking of svelte instead while discussing with @dominikg. Because if we go down that path (force everything to be namespace), it would be cool to have the svelte config to only contain export const namespace = {} instead of export default, since that would make type-ing the options easier, instead of combining them to one big object.

But re common keys, compilerOptions and preprocess are definitely needed. A hot or hmr option would be great too, so that makes 3 standardized options.

Re snowpack input, yeah it makes sense to rename that to extensions. Snowpack's plugin follows rollup-style too.

@benmccann
Copy link
Member

Actually, snowpack is pretty weird. It's called input if you pass it directly or extensions if you read it from the config file. It's passed to a snowpack option called resolve.input, so I can see why they called it input

I think the problem with having a generic key like build is where do you put bundler-specific stuff? Would Rollup's include and exclude go under build or under rollup key? If the former, it seems like a lot of stuff could end up getting shoved in there and keys would have different behavior depending on the bundler. If the latter, then keys would be split between build and rollup, etc. I think I'm leaning more towards it being better not to have shared options and to use rollupPlugin, vitePlugin, snowpackPlugin, webpackPlugin, etc. instead

it would be cool to have the svelte config to only contain export const namespace = {} instead of export default, since that would make type-ing the options easier

That's not a bad idea. We can do it even without shared options

@bluwy
Copy link
Member

bluwy commented Aug 24, 2021

I think keys split between build and rollup would be fine to be more explicit. build can include anything that affects the compiled svelte component output, so it should be bundler-agnostic. Anything else would be in their own namespaced options.

We might also need to take into account of svelte-vscode reading the config too. If that requires a separate vscode key, we'll likely have to declare compilerOptions and preprocess twice.

@dominikg
Copy link
Member Author

would it be better to create an RFC for it? This affects many packages in the svelte-sphere and we also should look at how everyone can introduce/migrate to that in a user-friendly (non-breaking?) way.

Other things to note: at least kit also reads "extensions" in the root of config. https://github.com/sveltejs/kit/blob/b77201613c62d9afabf9c4135d6f18336ce9409a/packages/kit/src/core/config/options.js#L13

preprocess and compilerOptions as shared options make sense as they directly affect compiler output and this is essential for all involved. I think keeping them in root would cause less work for implementation and adoption.

@bluwy
Copy link
Member

bluwy commented Aug 24, 2021

Yeah. I think this should be an RFC to gather more feedback from other bundler/framework maintainers as well. Would be nice to ping them too.

@benmccann
Copy link
Member

RFC makes sense. It might be good to discuss at a maintainer's meeting first since there's been a number of options discussed to see which are the most popular / unpopular

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants