-
Notifications
You must be signed in to change notification settings - Fork 151
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
feat(babel): provide caller information to Babel #449
Conversation
@@ -20,6 +20,14 @@ const transformer: Transformer<Options.Babel> = async ({ | |||
minified: false, | |||
ast: false, | |||
code: true, | |||
caller: { | |||
name: 'svelte-preprocess', | |||
supportsStaticESM: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've sort of only covered this option with a test. The rest is analogous and I'm not sure if it's super important to cover all of that with explicit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the defaults for that Babel config? In other words, is this a hidden breaking change because things will be transpiled differently now? If yes, are is the things that will be transpiled differently now the desired outcome since it doesn't make sense anyway for Svelte JavaScript code to be preprocessed to CJS and this can be seen as a fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the defaults for that Babel config?
Those describe language features supported by the "caller". Like, for example, in the case of @rollup/plugin-babel
it doesn't make sense for Babel to transpile features that are supposed to be handled by Rollup itself. This was misconfigured by people all the time in the past and that's why the Babel team has introduced this caller
option. It's a way to provide a tool/caller-based information, it's acting as an implicit config for Babel (sort of).
is this a hidden breaking change because things will be transpiled differently now?
In certain situations - yes, this could be treated as a breaking change. It's hard for me to imagine when people would actually prefer the old output over the new one. This is a subtle matter though - so it's up to you to decide if this can be merged today or if this has to wait for the next major version of this package.
If yes, are is the things that will be transpiled differently now the desired outcome since it doesn't make sense anyway for Svelte JavaScript code to be preprocessed to CJS and this can be seen as a fix?
Yes, sort of - I could see this being a fix but YMMV. Each of the properties that were added here should be evaluated separately. I think that all of them can be treated as "fixes" as it's rarely desired for those things to be transpiled on the Babel level. When those are transpiled by Babel Rollup cannot handle them natively as it won't attempt to understand the transpiled output. And it seems to me that in 99% (scientific number 😉 ) of the use cases one wants to handle the code authored with those language featured to Rollup so it can use them efficiently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so let's go through them:
supportsStaticESM
-> I assume this meansimport {foo} from 'bar';
imports. This should betrue
by default, in fact there's sometimes confusion when the tsconfig is misconfigured andrequire
s start appearing in the preprocessed output, the Svelte compiler can't deal with thatsupportsDynamicImport
-> I assume thisimport('..')
. Should betrue
by default. Svelte files are in loaded in the browser, I assume this is transpiled to somerequire
stuff again and this doesn't make sense in a browser environment.supportsTopLevelAwait
-> Svelte files don't support top level await as the code should run top to bottom in one sweep. Now the questions is what does this get transpiled to. Is it a promise that is not awaited (likefoo.then(() => {..all the stuff below the original await}
)? If yes it might make sense to not set this by default. But one could also argue that this should not be transpiled and instead show a Svelte compiler error so people are aware of this caveat. This is one could be a breaking change in some very rare cases. @kaisermann what are your thoughts on this?supportsExportNamespaceFrom
-> I think it doesn't hurt if this is transpiled during preprocessing already. We can't assume thatsvelte-preprocess
is always used in the context ofrollup
(could bevite
,webpack
, etc) and I don't know how they handle that. Do you know if all three of the mentioned tools transpile this correctly themselves? If they don't I think it makes sense to not set this totrue
by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supportsTopLevelAwait -> Svelte files don't support top level await as the code should run top to bottom in one sweep. Now the questions is what does this get transpiled to.
It won't compile by default - it errors with "'await' is only allowed within async functions". If the Svelte compiler throws a better error for this it might still be worth accepting it here, at this stage. I don't have good means to verify this right now though.
supportsExportNamespaceFrom -> I think it doesn't hurt if this is transpiled during preprocessing already. We can't assume that svelte-preprocess is always used in the context of rollup (could be vite, webpack, etc) and I don't know how they handle that. Do you know if all three of the mentioned tools transpile this correctly themselves? If they don't I think it makes sense to not set this to true by default.
Vite is Rollup-based and Rollup supports this since 1.26.0. Support in webpack for that was implemented here and it got shipped in v5.0.0-beta.21.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't compile by default - it errors with "'await' is only allowed within async functions". If the Svelte compiler throws a better error for this it might still be worth accepting it here, at this stage. I don't have good means to verify this right now though.
Good to know, thanks! I think true
by default is good then 👍
Vite is Rollup-based and Rollup supports this since 1.26.0. Support in webpack for that was implemented here and it got shipped in v5.0.0-beta.21.
Thanks for the pointers! Webpack 4 is quite popular still so I think it's better to not enable this by default then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i've commented this out and added a comment about this and about supportsTopLevelAwait
. Let me know what do you think about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dummdidumm @Andarist Thanks for the very detailed discussion 🙏 I will give this some attention now and test it in a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I do agree to leave syntax errors with svelte and let babel skip transformation. This means the code being sent to the compiler is more similar to the one that was written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Waiting on review by @kaisermann before merging this because this is an area he might know more about
Released in |
This PR provides
options.caller
to Babel when transforming files, it's what@rollup/plugin-babel
does here and what webpack'sbabel-loader
does hereWhen this is configured on the caller's side (
svelte-preprocess
in this scenario) then@babel/preset-env
won't touch certain language features by default - they will still be transpiled if there is an explicit configuration to do so.This allows for people to maintain a single Babel config where each tool can opt-out of certain transforms. For instance, with the config used in the added test modules would be transpiled to CJS when running in Jest but they will be preserved when transpiling through
svelte-preprocess
so Svelte's compiler will see them, instead of CJS'srequire
.Tests
npm test
oryarn test