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

Feature/esm as default transformer #101

Conversation

sebastianrothe
Copy link
Collaborator

@sebastianrothe sebastianrothe commented Feb 13, 2022

FYI: this is based on #100. So if #100 gets merged first, the changes here a much smaller.

This change makes the ESM async transformer the default. It also adds an error, if it used in the wrong context (e.g. async in CJS). This should fix most of the problems (#97 #91 #86 #72), because it will let Jest use the correct transformer.

It adds documentation with a manual override for all users, who still have to use CJS.

adyshev and others added 23 commits January 29, 2022 18:39
… be of type string. Received an instance of URL
- remove cjs export
- remove main, because node <12 is not supported anymore
- add check for module system and correct transformer
- make it clearer, that esm is the default and cjs has to be set manually
- v12 is supported until April 2022
- remove v12
- add v17 for CI
@sebastianrothe sebastianrothe added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 14, 2022
@sebastianrothe sebastianrothe requested review from mihar-22 and benmccann and removed request for benmccann February 14, 2022 19:41
@sebastianrothe
Copy link
Collaborator Author

src/transformer.js Outdated Show resolved Hide resolved
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@benmccann
Copy link
Collaborator

Sharing something I learned, which is the reason you had to write the PR this way, is that jest always prefers the CJS version via its requireOrImportModule method: https://github.com/jestjs/jest/blob/2499cf2a3a75824fd0c9b454baa95fc75929f086/packages/jest-transform/src/ScriptTransformer.ts#L24

With Svelte 4, CJS won't work, so I think this PR is a good idea. Svelte 4 ships a CJS version of the compiler for use in eslint, but it doesn't ship a CJS version of the runtime, so I don't think it'd be possible to run tests in CJS mode. It might make sense to avoid removing CJS support just yet so that people can still use CJS with Svelte 3, so I think I like just making ESM the default as you've done here and leaving CJS support in place while we see how much trouble or success people have in migrating. If it turns out we really screwed things up by removing CJS support too early in Svelte, it would make it slightly easier to add back to already have CJS support here - though I hope that wouldn't be necessary.

Somewhat surprising to me, it turns out that Jest is still used much more than Vitest or Playwright are with Svelte eventhough the latter two are the ones we offer to setup people up with when setting up new SvelteKit projects. Hopefully Svelte and svelte-jester dropping CJS won't break too many people too badly, but I do know jest's ESM support is immature (not entirely their fault as they depend on node who depend on v8).


if (debug) {
console.debug(`Running svelte-jester-transformer async in mode ${currentFileExtension}.`)
}

if (!preprocess) {
return compiler('esm', options, filename, source)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to omit the format option with Svelte 4 or else users will get errors printed to the console. I think we can do something like import { VERSION } from 'svelte'; to check what version users are on. @sveltejs/vite-plugin-svelte does this

@benmccann
Copy link
Collaborator

I think it's easier to start fresh with this PR rather than address all the merge conflicts, so I'm going to go ahead and close it. I sent #133 with the core changes from this PR, so please give that one a look if you've got a minute!

@benmccann benmccann closed this Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants