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

Fix scopehositing with nested dynamic imports #2712

Merged
merged 1 commit into from
Mar 3, 2019

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Mar 3, 2019

↪️ Pull Request

Closes #2300, @garthenweb

Probably introduced by #1682

When the assets are iterated on JSConcatPackager.start

for (let asset of this.bundle.assets) {
// If this module is referenced by another JS bundle, it needs to be exposed externally.
let isExposed = !Array.from(asset.parentDeps).every(dep => {
let depAsset = this.bundler.loadedAssets.get(dep.parent);
return this.bundle.assets.has(depAsset) || depAsset.type !== 'js';
});
if (
isExposed ||
(this.bundle.entryAsset === asset &&
this.bundle.parentBundle &&
this.bundle.parentBundle.childBundles.size !== 1)
) {
this.exposedModules.add(asset);
this.needsPrelude = true;
}
this.assets.set(asset.id, asset);

they are also added to the assets Map. For "normal" bundles bundle-loader is added here, but for dynamically imported bundles only actual static imports is added.
For dynamically imported bundles which need also need the bundle-loader, it gets added here (called by addAsset)
async addBundleLoader(bundleType, parentAsset, dynamic) {
let loader = this.options.bundleLoaders[bundleType];
if (!loader) {
return;
}
let bundleLoader = this.bundler.loadedAssets.get(
require.resolve('../builtins/bundle-loader')
);
if (!bundleLoader && !dynamic) {
bundleLoader = await this.bundler.getAsset('_bundle_loader');
}
if (bundleLoader) {
// parentAsset.depAssets.set({name: '_bundle_loader'}, bundleLoader);
await this.addAssetToBundle(bundleLoader);
, but addAssetToBundle didn't add it to the assets map, resulting in this error:

Cannot read property 'depAssets' of undefined 
    at JSConcatPackager.resolveModule (<work_space>/node_modules/parcel-bundler/src/packagers/JSConcatPackager.js:551:19)
    at CallExpression (<work_space>/node_modules/parcel-bundler/src/scope-hoisting/concat.js:152:28)
    at NodePath._call (<work_space>/node_modules/@babel/traverse/lib/path/context.js:53:20)
    at NodePath.call (<work_space>/node_modules/@babel/traverse/lib/path/context.js:40:17)
    at NodePath.visit (<work_space>/node_modules/@babel/traverse/lib/path/context.js:88:12)
    at TraversalContext.visitQueue (<work_space>/node_modules/@babel/traverse/lib/context.js:118:16)
    at TraversalContext.visitSingle (<work_space>/node_modules/@babel/traverse/lib/context.js:90:19)
    at TraversalContext.visit (<work_space>/node_modules/@babel/traverse/lib/context.js:146:19)
    at Function.traverse.node (<work_space>/node_modules/@babel/traverse/lib/index.js:94:17)
    at NodePath.visit (<work_space>/node_modules/@babel/traverse/lib/path/context.js:95:18)

💻 Examples

Fix scopehoisting for situations like this:

bundle   ----dynamic-import--->   bundle   ----!!dynamic-import!!--->   bundle

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@garthenweb
Copy link
Contributor

Ha, I thought about these lines as well but was not able to put the pieces together ;)
I can prove that this solves it for my example as well, really cool that you found and fixed it! 🎉

@devongovett devongovett merged commit 37e22d3 into master Mar 3, 2019
@mischnic mischnic deleted the scope-hoisting-nested-dynamic branch March 3, 2019 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot read property 'depAssets' of undefined
3 participants