Don't AMD transform non-module scripts. #146
Conversation
packages/build/src/js-transform.ts
Outdated
| plugins.push(...babelTransformModulesAmd); | ||
| if (options.transformModulesToAmd && options.transformImportMeta === false) { | ||
| throw new Error( | ||
| 'Cannot use transformModulesToAmd without transformImportMeta.'); |
There was a problem hiding this comment.
Can we just turn on transformImportMeta when transformModulesToAmd is specified? Like, at the initial place where we're handed options? Or is this a thing that indicates a bug in polymer-build?
There was a problem hiding this comment.
Done. We now always turn on the import.meta transform when the AMD transform happens, regardless of the transformImportMeta option. Previously it would be auto-set only if you left it undefined, but now you can set it to false explicitly and we will override. Previously we threw because it's non-sensical to ask for AMD but not import.meta (but note that the inverse is fine), but maybe just overriding is more reasonable.
I also tried to make the code around this option more clear. I'm not sure if I succeeded.
packages/build/src/js-transform.ts
Outdated
| } | ||
|
|
||
| if (doBabelTransform) { | ||
| js = babelCore.transformFromAst(ast, js, {presets, plugins}).code!; |
There was a problem hiding this comment.
code! looks suspicious here. The library is telling us that it could be null, and we're overriding it. If the library is right, we're kicking the exception down the road to a less clear place.
WDYT about checking and throwing instead?
There was a problem hiding this comment.
Done. I doubt it can happen without Babel itself throwing, probably just a weakness in the Babel typings (the thing it returns is used by a bunch of functions and has optional everything https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/babel-core/index.d.ts#L172), but I can't say for sure, so added a check and throw.
packages/build/src/js-transform.ts
Outdated
| ast = babylon.parse(js, { | ||
| // TODO(aomarks) Remove any when typings are updated for babylon 7. | ||
| sourceType: options.transformModulesToAmd === 'auto' ? | ||
| 'unambiguous' as any : |
There was a problem hiding this comment.
Neat! We should do this trick in the analyzer too, to avoid double-parsing module files.
packages/build/src/js-transform.ts
Outdated
| js = babelCore.transformFromAst(ast, js, {presets, plugins}).code!; | ||
| const result = babelCore.transformFromAst(ast, js, {presets, plugins}); | ||
| if (result.code === undefined) { | ||
| throw new Error('Unexpected undefined code result from Babel.'); |
There was a problem hiding this comment.
Maybe: Babel transform failed: resulting code was undefined
We now pay more attention to whether an HTML script has type=module via the HTML splitter, and only run the AMD transform on those that do. For external scripts, we now only run the AMD transform if the file looks like a module based on whether it has import or export statements. Babel 7 can do this for us with the "unambiguous" sourceType.
aomarks
force-pushed
the
dont-amd-non-module
branch
from
3ac861b
to
847a363
Compare
We now pay more attention to whether an HTML script has
type=modulevia the HTML splitter, and only run the AMD transform on those that do.For external scripts, we now only run the AMD transform if the file looks like a module based on whether it has import or export statements. Babel 7 can do this for us with the "unambiguous"
sourceType.Fixes https://github.com/Polymer/polymer-cli/issues/994