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 missing child bundles throwing an error #378

Merged
merged 2 commits into from
Sep 17, 2020

Conversation

dabbott
Copy link
Contributor

@dabbott dabbott commented Aug 29, 2020

This fixes an issue introduced in #376, where getChildAssetBundles sometimes doesn't find the given statAsset.name and returns undefined, which later causes an error.

This probably isn't a great fix, and I'm not sure specifically how to reproduce the issue, but it seems harmless enough that I'll be using it for now at least 😅

@jsf-clabot
Copy link

jsf-clabot commented Aug 29, 2020

CLA assistant check
All committers have signed the CLA.

@dabbott
Copy link
Contributor Author

dabbott commented Aug 29, 2020

@jsf-clabot rerun

@valscion valscion closed this Aug 31, 2020
@valscion valscion reopened this Aug 31, 2020
@valscion
Copy link
Member

Thanks! Would you be able to setup a test with stats.json that breaks, similar to files in here? https://github.com/webpack-contrib/webpack-bundle-analyzer/tree/master/test/stats

Those files are used by tests defined in https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/test/analyzer.js

Copy link
Collaborator

@th0r th0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valscion let's not merge it without a test please.

@valscion
Copy link
Member

valscion commented Sep 1, 2020

@valscion let's not merge it without a test please.

👍 sounds good to me!

@masterkidan
Copy link
Contributor

@dabbott : It would be great if you can debug a bit deeper, my understanding is that we eliminate assets generated by plugins like HtmlWebpackPlugin in the regex/assetfilter (only js assets are analyzed). It looks like in your webpack config something with HtmlWebpackPlugin is generating js assets, can you share the config so I can understand the issue a bit better?

@dabbott
Copy link
Contributor Author

dabbott commented Sep 3, 2020

@masterkidan Thanks for checking this out. Here's my webpack config: https://github.com/dabbott/react-native-web-player/blob/58e49839516a32a91267510b550823e644b72364/webpack/webpack.config.js.

If you'd like, you should be able to pull down that react-native-web-player/webpack-bundle-analyzer branch and run yarn build to see the error (if you have webpack-bundle-analyzer linked locally so you're pointing to master). I don't know how webpack assets are organized at all really, so I probably won't be much help in making a better fix, but if you have suggestions or a fix to test I can definitely try it out.

@masterkidan
Copy link
Contributor

masterkidan commented Sep 5, 2020

@dabbott : I think this may be because of the fact your template is an 'ejs' file. This is not something that gets filtered out by the regex, but the template compilation is a child compilation, I think a slightly better way would be to check why
the following bit of code in analyzer.js

// Picking only `*.js or *.mjs` assets from bundle that has non-empty `chunks` array
  bundleStats.assets = _.filter(bundleStats.assets, asset => {
    // Removing query part from filename (yes, somebody uses it for some reason and Webpack supports it)
    // See #22
    asset.name = asset.name.replace(FILENAME_QUERY_REGEXP, '');

    return FILENAME_EXTENSIONS.test(asset.name) && !_.isEmpty(asset.chunks) && isAssetIncluded(asset.name);
  });

is not filtering out the asset, and fix it here instead :) .

You can use https://github.com/masterkidan/worker-loader-no-stats as a base and add a simple index.ejs template with a WebpackHtlmPlugin to process it.... I am not sure when I will be able to get to it on my own.

@dabbott
Copy link
Contributor Author

dabbott commented Sep 9, 2020

Ok! I was able to track this down. This happens when workers contain dynamic imports with import('./some-dep.js'). I added a test case (generated via https://github.com/dabbott/worker-loader-dynamic-imports).

I think this is probably in an OK state to merge, and @masterkidan if you have time you could do a better fix in the future.

@th0r
Copy link
Collaborator

th0r commented Sep 10, 2020

@dabbott so, what is the actual reason for the exception? What asset it tries to find but it's not there?

@dabbott
Copy link
Contributor Author

dabbott commented Sep 10, 2020

We're looking for "1.bundle.worker.js" as assetName in:

function getChildAssetBundles(bundleStats, assetName) {
  return _.find(bundleStats.children, (c) =>
    _(c.assetsByChunkName)
      .values()
      .flatten()
      .includes(assetName)
  );
}

That name doesn't exist in assetsByChunkName. (Maybe cause it's not a named chunk? It does exist in c.chunks I believe)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants