Default interop missing when importing a CommonJS module#7991
Merged
Conversation
thebriando
commented
Apr 21, 2022
lettertwo
reviewed
Apr 22, 2022
Benchmark ResultsKitchen Sink ✅
Timings
Cold Bundles
Cached Bundles
React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold Bundles
Cached BundlesNo bundle changes detected. |
Co-authored-by: Eric Eldredge <lettertwo@gmail.com>
Co-authored-by: Eric Eldredge <lettertwo@gmail.com>
mischnic
reviewed
Apr 22, 2022
Member
|
Looks like there's a problem in the transformer: import b from './b';
const foo = b.foo;
const bar = require('./b').foo;
output = foo + bar;becomes const $aa66d08168f59e76$var$foo = $aa66d08168f59e76$import$61a9e60a12024cdc$2e2bcd8739ae039.foo;
import "aa66d08168f59e76:./b";
var $aa66d08168f59e76$require$bar = $aa66d08168f59e76$import$61a9e60a12024cdc$6a5cdcad01c973fa;
output = $aa66d08168f59e76$var$foo + $aa66d08168f59e76$require$bar;so both the require and the import usage get attributed to the same dependency. Furthermore, this dependency has
And indeed for a (And there's probably a situation right now where the inverse happens, that |
Co-authored-by: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com>
…s match while iterating
thebriando
force-pushed
the
bdo/default-interop
branch
from
7b6ab7a to
7bf63b3
Compare
thebriando
commented
May 9, 2022
| ((dep.meta.kind === 'Require' || | ||
| dep.meta.kind === 'Import' || | ||
| dep.meta.kind === 'DynamicImport') && | ||
| dep.meta.kind !== kind) |
Contributor
Author
There was a problem hiding this comment.
dep.meta.kind is a DependencyKind rather than a ImportKind so I wasn't really sure what to do here..
thebriando
changed the title
mischnic
reviewed
May 12, 2022
Comment on lines
+729
to
+739
| for (let dep of nullthrows(deps.get(source))) { | ||
| if ( | ||
| (dep.meta.kind === 'Require' || | ||
| dep.meta.kind === 'Import' || | ||
| dep.meta.kind === 'DynamicImport') && | ||
| dep.meta.kind !== kind | ||
| ) { | ||
| continue; | ||
| } | ||
| dep.symbols.set(imported, local, convertLoc(loc)); | ||
| } |
Member
There was a problem hiding this comment.
Could this be simplified to just
let dep = nullthrows(deps.get(source)).find(d => dep.meta.kind === kind)?
Contributor
Author
There was a problem hiding this comment.
So there's a case where dep.meta.kind is Export and kind is import and I think we should be setting the symbols in this case which is why I had the condition to continue if there's a mismatch and if dep.meta.kind is an ImportKind
Member
There was a problem hiding this comment.
So this would be the case where you have both reexports and imports of the same specifier in a single asset. But it looks like currently import always wins (so dep.meta.kind will be import in asset.getDependencies()). (Kind of strange anyway that we have dep.meta.kind anyway. AFAICT dep.specifierType would be sufficient for the packager, then there's only be esm or cjs)
And in this loop, kind is always Import or Require.
Contributor
Author
There was a problem hiding this comment.
Hmm in this test it looks like Export wins for dep.meta.kind so the symbols wouldn't get set if we have that single condition.
dep.sourcePath: /Users/bdo/parcel/packages/core/integration-tests/test/integration/scope-hoisting/es6/re-export-import/d.js
dep.meta.kind: Export
mischnic
reviewed
May 12, 2022
Comment on lines
+742
to
+745
| for (let dep of nullthrows(deps.get(source))) { | ||
| if (local === '*' && imported === '*') { | ||
| dep.symbols.set('*', '*', convertLoc(loc), true); | ||
| } else { |
Member
There was a problem hiding this comment.
I think this shouldn't be set for all dependencies with that specified for only for the Import one?
Contributor
Author
There was a problem hiding this comment.
So we shouldn't set the symbols if kind is Import?
Member
There was a problem hiding this comment.
This loop is applying hoist_result.re_exports. Which can only come from ESM export .. from ...; statements. So here we're only interested in the ESM dep . For example in
export {a as b} from "foo";
const {x} = require("foo");
console.log(x)
devongovett
reviewed
May 17, 2022
| if (!deps.has(dep.meta.placeholder ?? dep.specifier)) { | ||
| deps.set(dep.meta.placeholder ?? dep.specifier, []); | ||
| } | ||
| deps.get(dep.meta.placeholder ?? dep.specifier)?.push(dep); |
Member
There was a problem hiding this comment.
A possible alternative way of implementing this could be to make the map keyed by (dep.meta.placeholder ?? dep.specifier) + dep.specifierType, rather than an array of dependencies per specifier.
Contributor
Author
There was a problem hiding this comment.
I think the main reason why I didn't add the kind to the key is because we only want to do the ImportKind specific stuff when we are looking at hoist_result.imported_symbols. Keying by specifier + specifierType would mean that we would have to change all the other things in the hoist_result to track the kind, but I'm not sure if we should do that.
mischnic
reviewed
May 23, 2022
Comment on lines
764
to
768
| for (let specifier of hoist_result.wrapped_requires) { | ||
| let dep = deps.get(specifier); | ||
| if (!dep) continue; | ||
| dep.meta.shouldWrap = true; | ||
| for (let dep of nullthrows(deps.get(specifier))) { | ||
| dep.meta.shouldWrap = true; | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
dep.meta.shouldWrap will get set for every dependency with that specifier. But it should have been for the require dep only.
Member
|
I still find the approach of adding for loops problematic. I can't think of a case where an entry from |
Contributor
Author
|
So in this case We get this error because there's two deps with './b' as the specifier but we only wrap the |
Member
This loop replaces all occurences of imports and requires as output by the transformer. But there's once again the problem that there's no difference between the twin ESM and CJS deps: there are two identical specifiers here, where the first should refer to the ESM dep and the second to the CJS dep. import "b49a022a1bb2bce3:./b";
const $b49a022a1bb2bce3$export$6a5cdcad01c973fa = $b49a022a1bb2bce3$import$61a9e60a12024cdc$2e2bcd8739ae039.foo; // <-- missing default interop
import "b49a022a1bb2bce3:./b";
const $b49a022a1bb2bce3$export$d927737047eb3867 = (()=>$b49a022a1bb2bce3$import$61a9e60a12024cdc$6a5cdcad01c973fa
)();
output = $b49a022a1bb2bce3$export$6a5cdcad01c973fa + $b49a022a1bb2bce3$export$d927737047eb3867;So the callback runs with |
Contributor
Author
|
So how does parcel/packages/transformers/js/src/JSTransformer.js Lines 754 to 758 in 49136f0 |
Member
|
Trying to summarize the conversation:
|
…hashed with DependencyKind's
lettertwo
approved these changes
Jul 1, 2022
|
Thanks for fixing! Can we ge a release with this fix? I think we are affected in diegomura/react-pdf#1915 |
8 tasks
Contributor
Author
|
@carlobeltrame Yes! We will try to get one out soon. cc: @devongovett |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
↪️ Pull Request
There's a case where we don't add a default interop for a cjs module in
getSymbolResolutionThis causes an error at runtime since it tries to call default which would be undefined in this case
💻 Examples
Repl
🚨 Test instructions
✔️ PR Todo