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 for ESM configs #29

Closed
dummdidumm opened this issue Apr 11, 2021 · 10 comments
Closed

Support for ESM configs #29

dummdidumm opened this issue Apr 11, 2021 · 10 comments
Labels
enhancement New feature or request

Comments

@dummdidumm
Copy link
Member

Describe the feature
Support loading ESM config files. Currently only CommonJS svelte.config files are supported because the code uses require to load it.

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).

A Usecase
Support ESM svelte.config.js in SvelteKit

Alternatives
none

Additional context
Blocker for sveltejs/kit#936

@dummdidumm dummdidumm added the enhancement New feature or request label Apr 11, 2021
@dominikg
Copy link
Member

Needs support for async config from vite, this has been merged but not released vitejs/vite#2758

Also does this mean that going forward there will be no svelte.config.cjs and we should discourage its use or are both allowed?

And while we're at it, whats the list of locations vite-plugin-svelte should look for svelte.config.c?js ? (see #23 for context)

@dominikg
Copy link
Member

Update, async config hook might be a better place: vitejs/vite#2800

@dummdidumm
Copy link
Member Author

Both cjs and js should be supported. It should check .js first, then .cjs to be in line with the order how language-tools and SvelteKit (will) resolve it.

Regarding "where to look": I think this was never specified. But we should, at some point, to save us some maintenance 😄

@dominikg
Copy link
Member

Initial code here https://github.com/sveltejs/vite-plugin-svelte/compare/feat/async-load-config-esm needs to wait until vite 2.16 is released and peer dependency updated.

should we try to get rid of everything non-esm and make vite-plugin-svelte itself a type: module too?

Here's another require call:

, anything else?

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 19, 2021

You also need to replace the require-relative dependency I think.

Making it a type: module may be a little much though, I don't know who else uses this plugin in which configs. If those are commonjs-configs they can no longer use your package. If you are certain that's not the case then go for it I'd say. If not, you could also make it dual both supporting ESM and commonjs through separate entry points if you want to put that extra work on you (I don't know much about that topic).

@janosh
Copy link

janosh commented Apr 24, 2021

Very interested in this issue. Anything I can do to help?

@dominikg
Copy link
Member

unfortunately jest doesn't really support esm, so a fallback to cjs is needed.

@dummdidumm would it be ok to require the cjs extension or fallback to require if import throws?

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 24, 2021

Fine with me, if import() throws for real users later on there's not much you can do anyway. Error message might be confusing though, maybe the error message of the import() should be "saved" and thrown instead if require fails, too.

dominikg added a commit that referenced this issue Apr 27, 2021
* feat(vite-plugin-svelte): support esm in svelte.config.js (see #29)

* update vite to 2.2.1 and update other dependencies

* chore: update dependencies

* test: improve stability

* feat: add configFile option; use require to load cjs config

* also try to load svelte.config.mjs

* chore: update dependencies

* improve changesets
@dominikg
Copy link
Member

released in 1.0.0-next.8

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 28, 2021

I encountered a bug on windows. It tells me that the given path is not a valid scheme. Seeing that I remembered that I had the same issue in language-tools and what I did was wrapping the path with pathToFileUrl - sorry for not catching that earlier.

Also see nodejs/node#31710

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

No branches or pull requests

3 participants