stefanpenner / broccoli-concat-analyser

MIT License
126 stars 14 forks source link

Excludes packages imported using ember-auto-import #34

Closed patocallaghan closed 4 years ago

patocallaghan commented 6 years ago

It appears that broccoli-concat-analyser doesn't include packages which have been imported using ember-auto-import. I'm unsure whether the issue is in this addon or ember-auto-import but thought I'd open it here first.

To reproduce the issue I've made a sample repo where I've done the following:

  1. Installed ember-auto-import
  2. Installed lodash
  3. Imported lodash into app/app.js

Running ember asset-sizes we get the following size vendor.js

File sizes:
 - dist/assets/vendor.js: 3.2 MB (683.25 KB gzipped)

But broccoli-concat-analyser outputs the following size for vendor.js

 - dist/assets/vendor.js: 2.65 MB (218.44 KB gzipped)

image

There looks to be a large discrepancy there. Looking at the generated stats it appears the auto-imported lodash has been excluded.

image

The reproduction repo is here and I've committed the generated broccoli-concat-analyser stats

simonihmig commented 6 years ago

Thanks for pointing this out!

I quickly skimmed through the ember-auto-import code, and found this interesting explanation (here):

This is a fairly specialized broccoli transform that we use to get the output of our webpack build added to the ember app. Mostly it's needed because we're forced to run quite late and use the postprocessTree hook, rather than nicely emit our content as part of treeForVendor, etc, which would be easier but doesn't work because of whack data dependencies in new versions of ember-cli's broccoli graph.

It seems the bundled JavaScript by ember-auto-import is added to the final bundle after broccoli-concat has run, so the latter does not know anything about those files. @ef4 can you confirm?

The analyzer capabilities here are pretty much coupled to broccoli-concat, which worried me a bit for a while. This seems to be the first real manifestation of this problem. But also without this case, given the new Packager RFC, we cannot assume that everything runs through broccoli-concat, right? AFAICT other libs like rollup will probably used to bundle modules in the future, to support tree shaking etc.

Maybe we should have tools that are agnostic of the bundler used, something like https://github.com/danvk/source-map-explorer which only relies on sourcemaps?

@stefanpenner your thoughts?

ef4 commented 6 years ago

I don't think this is really a problem with broccoli-concat-analyzer. I think the stage where it analyzes is appropriate.

It would be much better if auto-import could go back to emitting code in treeForVendor. That's what it did originally, and that was easier to implement, more robust, and plays better with other tools (including this one). But I was forced to switch to a much hackier solution because the packager work in current ember-cli seriously messes up the data flow dependency graph.

I hope ember-cli will sort that out and I can go back to using treeForVendor.

simonihmig commented 6 years ago

Thanks for the quick feedback!

I hope ember-cli will sort that out and I can go back to using treeForVendor.

Agree. Assuming that is the case, how does the emitted vendor tree look like? Looking at this previous code it would be just one big .js file that webpack already pre-bundled. If so this would be opaque to broccoli-concat i.e. it would not be able to "see" the individual modules it contains, thus just one "big" ember-auto-import/app.js would show up in the analyzer, right? Still not ideal! 🤔

ef4 commented 6 years ago

True. Maybe a sourcemap-based solution would be better for that.

patocallaghan commented 6 years ago

Thanks for all the context guys, makes sense. I'm happy if you want to go ahead and close the issue.

allthesignals commented 5 years ago

What's bizarre is that this was working before:

image

mapbox-gl is imported via ember-auto-import. Maybe I had upgraded to some change that nixed this ability.

ef4 commented 5 years ago

Or maybe before you had two copies of mapbox-gl.js somehow. 😛

allthesignals commented 5 years ago

@ef4 that's terrifying! 😂

samselikoff commented 5 years ago

just got bit by this!

jfrux commented 5 years ago

Maybe I missed the solution but... if Ember is building with 1.5MB but this tool only outputs 446KB for vendor, what would be causing this? It seems this is a similar issue but I'm not sure?

ef4 commented 5 years ago

As a workaround, it's possible to get a separate report for just the ember-auto-imported things by adding webpack-bundle-analyzer directly to ember-auto-import's configuration:

autoImport: {
  webpack: {
    plugins: [
      new (require("webpack-bundle-analyzer")).BundleAnalyzerPlugin(),
    ],
  },
},

When you run ember s that will pop up a browser window showing everything that was auto-imported.

stefanpenner commented 4 years ago

broccoli-concat-analyser only provides insights into what broccoli-concat concats. So I'm not sure it is bug that other mechanisms of concatenation are not accounted for.

That being said, I do totally agree a holistic tool would be better, and as part of the embroider world, we most likely already have that (as @ef4 describes above)