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

Support esm config files #936

Merged
merged 6 commits into from
May 2, 2021
Merged

Support esm config files #936

merged 6 commits into from
May 2, 2021

Conversation

dummdidumm
Copy link
Member

This adds support for loading svelte.config.js - config files in ESM format, which is now supported by language-tools. It tries to load the .js format first and falls back to .cjs to ensure backwards compatibility; some people may also want to keep using cjs config files due to things like easier imports of JSON files. This is also the reason why there's no change in the starter template: its config contains an import of package.json, and JSON imports are hidden behind an experimental node flag for now.

Simon Holthausen added 2 commits April 9, 2021 17:25
This adds support for loading `svelte.config.js` - config files in ESM format, which is now supported by language-tools. It tries to load the `.js` format first and falls back to `.cjs` to ensure backwards compatibility; some people may also want to keep using cjs config files due to things like easier imports of JSON files. This is also the reason why there's no change in the starter template: its config contains an import of `package.json`, and JSON imports are hidden behind an experimental node flag for now.
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

The flag mentioned is --experimental-json-modules: https://nodejs.org/api/esm.html#esm_json_modules
It looks like it was added in Node 12: https://nodejs.medium.com/announcing-a-new-experimental-modules-1be8d2d6c2ff

The package.json import is used to import dependencies:

noExternal: Object.keys(pkg.dependencies || {})

The need to do that may change in the future? I'm not quite sure. Some discussion about it here: #904 (comment)

Anyway, this seems like a reasonable default for now I think

Thanks so much for getting this to be supported!! 🎉

@dummdidumm dummdidumm marked this pull request as draft April 11, 2021 12:58
@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 11, 2021

Converting this to a draft because I just noticed that this also needs an update in vite-plugin-svelte - @dominikg I think that this line needs to change to use import(..) . Note though that TS will transpile this to a require anyway, that's why you need to create a const dynamicImport = new Function('path', 'return import(path)'); function and use that one as a workaround (see microsoft/TypeScript#43329 for more info).
I created an issue in the vite-plugin-svelte repo for that.

@dummdidumm dummdidumm marked this pull request as ready for review April 30, 2021 05:52
@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 30, 2021

This is now ready to review. I'd like to get a manual test of this from a linux/mac user. I tested this on windows and it worked for me. To test, I scaffolded the starter template and replaced the cjs config with an esm config.

I did not change the starter template even if it doesn't have the package.json import anymore. I'd like to split this decision out into a separate PR. We then also need to update all docs with config examples and add a FAQ entry about the possibility to switch to cjs if needed, for example if you need to import the package.json into your config (loading JSON isn't supported in ESM yet).

@Conduitry
Copy link
Member

Would module.createRequire(import.meta.url)('...') be a valid workaround for JSON loading? If we want to be able to rewrite adapters as ESM, it'd probably be helpful if the config file loading them were also ESM. (How would using ESM adapters in a CJS config work? Would you export a promise resolving to the config? Do we want an extra await in the config loading code to support this?) To avoid complications should we just say that loaders should always be CJS?

@dummdidumm
Copy link
Member Author

Would module.createRequire(import.meta.url)('...') be a valid workaround for JSON loading?

Maybe? If that is the suggested way by node, then yeah, probably. But this is a thing the user then has to do in his file, there's no way to do this automatically

How would using ESM adapters in a CJS config work? Would you export a promise resolving to the config? Do we want an extra await in the config loading code to support this?

The only way to load ESM in CJS is to do the dynamic import() which returns a promise.

@Conduitry
Copy link
Member

Right, which is why I was saying their CJS config would have no choice but exporting a promise resolving to their config. Do we want to plan for that by having an extra await as appropriate where we load the config? Or do we want to push createRequire as the way to do this? Or do we want the adapter API to simply remain CJS?

const config_file_esm = path.join(cwd, 'svelte.config.js');
const config_file = fs.existsSync(config_file_esm)
? config_file_esm
: path.join(cwd, 'svelte.config.cjs');
const config = await import(url.pathToFileURL(config_file).href);
const validated = validate_config(config.default);
Copy link
Member

Choose a reason for hiding this comment

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

I think an extra await here would be all that would be needed if we wanted to support files that exported a promise resolving to the configuration.

@dummdidumm
Copy link
Member Author

I think having the config being asynchronous should be okay for Kit, the language-tools and the vite plugin. Since all of them use import() to load the config, it's already async so this shouldn't make a difference. But we should check first to make sure.

Either way, I want this to go into separate PRs. This one makes it possible to use ESM configs while not changing any defaults, and it is still possible to use CJS.

@benmccann benmccann merged commit 08ebcb5 into master May 2, 2021
@benmccann benmccann deleted the esm-config-support branch May 2, 2021 02:29
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

3 participants