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.

Sign up for GitHub

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

Jump to bottom

feat(babel): provide caller information to Babel #449

Merged
merged 5 commits into from Jan 17, 2022
Merged

feat(babel): provide caller information to Babel #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a44c325
feat(babel): provide caller information to Babel
Andarist Dec 21, 2021
d1a06d0
Update test/transformers/babel.test.ts
dummdidumm Dec 22, 2021
803626b
Skip providing `supportsExportNamespaceFrom: true` for now
Andarist Dec 22, 2021
620b24a
add comment for `supportsTopLevelAwait: true`
Andarist Dec 22, 2021
7bf6b0c
chore: update src/transformers/babel.ts
kaisermann Jan 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/transformers/babel.ts
View file
Open in desktop
Expand Up @@ -20,6 +20,17 @@ const transformer: Transformer<Options.Babel> = async ({
minified: false,
ast: false,
code: true,
caller: {
name: 'svelte-preprocess',
supportsStaticESM: true,
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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 means import {foo} from 'bar'; imports. This should be true by default, in fact there's sometimes confusion when the tsconfig is misconfigured and requires start appearing in the preprocessed output, the Svelte compiler can't deal with that
  • supportsDynamicImport -> I assume this import('..'). Should be true by default. Svelte files are in loaded in the browser, I assume this is transpiled to some require 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 (like foo.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 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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

supportsDynamicImport: true,
// this isn't supported by Svelte but let it error with a good error on this syntax untouched
supportsTopLevelAwait: true,
// todo: this can be enabled once all "peer deps" understand this
// this syntax is supported since rollup@1.26.0 and webpack@5.0.0-beta.21
// supportsExportNamespaceFrom: true,
...options?.caller,
},
} as TransformOptions;

const result = await transformAsync(content, babelOptions);
Expand Down
33 changes: 33 additions & 0 deletions test/transformers/babel.test.ts
View file
Open in desktop
Expand Up @@ -40,4 +40,37 @@ $: bar = foo?.b ?? 120
$: bar = (_foo$b = foo == null ? void 0 : foo.b) != null ? _foo$b : 120;</script>"
`);
});

it('should not transpile import/export syntax with preset-env', async () => {
const template = `<script>
import foo from './foo'
$: bar = foo?.b ?? 120
</script>`;

const opts = sveltePreprocess({
babel: {
presets: [
[
'@babel/preset-env',
{
loose: true,
targets: {
esmodules: true,
},
},
],
],
},
});

const preprocessed = await preprocess(template, opts);

expect(preprocessed.code).toMatchInlineSnapshot(`
"<script>var _foo$b;

import foo from './foo';

$: bar = (_foo$b = foo == null ? void 0 : foo.b) != null ? _foo$b : 120;</script>"
`);
});
});