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

Support for tree shaking? #121

Open
alansouzati opened this issue Sep 5, 2018 · 14 comments
Open

Support for tree shaking? #121

alansouzati opened this issue Sep 5, 2018 · 14 comments

Comments

@alansouzati
Copy link

I'm new to the ember community. I'm trying to make sure my bundle is as small as it can be. I'm using a library that uses named exports so that webpack 4 handles tree shaking well.

I'm importing the library like this:

import { MyComponent } from "library-name"

Inside library-name there are a bunch of components being exported, and the final bundle includes everything.

I'm trying to figure out how to make sure my final bundle only include MyComponent.

I'm not entirely sure if this is a problem with ember-auto-import, since I'm new to the community.

@ef4
Copy link
Collaborator

ef4 commented Sep 6, 2018

Webpack will only shake out individual exports if the library in question marks itself as having no side effects. See https://github.com/webpack/webpack/blob/master/examples/side-effects/README.md

Beyond that, nothing special in ember-auto-import should be needed.

@alansouzati
Copy link
Author

Yeah I added sideEffets in the library and still no luck. Do you have an example of tree shaking working will with ember + ember auto import ?

@alansouzati
Copy link
Author

alansouzati commented Sep 6, 2018

also FYI, I frequently use babel-plugin-transform-imports which works well. But in ember it is not taking the config.

I installed the ember-cli-babel but it is not taking the config I'm passing:

let app = new EmberApp(defaults, {
    babel: {
      plugins: [
        ["transform-imports", {
          "library-name": {
            "transform": "unexisting/path/${member}"
          }
        }]
      ]
    }
  });

Ember builds successfully with this config. I was expecting it to fail since the transformation leads to an non-existing path.

To prevent big bundles, I'm trying to transform:

import { MyComponent } from 'library-name'

to

import { MyComponent } from 'library-name/es6/components/MyComponent'

@nag5000
Copy link

nag5000 commented Mar 2, 2020

I have the same problem with date-fns 2.x.

From docs (https://date-fns.org/v2.10.0/docs/ECMAScript-Modules):

date-fns v2.x provides support for ECMAScript Modules that enables tree-shaking for bundlers, like rollup.js and webpack.

If you have tree-shaking enabled in your bundler, just import functions normally:
import { format, parse } from 'date-fns'

When I do import like that import { isAfter } from 'date-fns';, the final app bundle includes all date-fns lib.

sideEffects is false: https://unpkg.com/date-fns@2.10.0/isAfter/package.json, https://unpkg.com/date-fns@2.10.0/esm/isAfter/package.json

@bobisjan
Copy link
Contributor

👋, I also saw the same problem with date-fns today.

I've found that it has something with Babel via ember-cli-babel and compiling modules, when I turned off compiling modules, then tree shaking worked as expected (but the overal output is broken).

// ember-cli-build.js
'use strict';

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    'ember-cli-babel': {
      compileModules: false
    }
  });
  return app.toTree();
};

It looks like webpack sees already compiled output without ES imports and thus it can not do tree shaking. @ef4, what do you think?


It is possible to use "full" paths like import addDays from 'date-fns/addDays'; to workaround the issue and have tree shaking.

@ef4
Copy link
Collaborator

ef4 commented Mar 27, 2020

That is interesting! We are supposed to already be turning that off here (for stage1 compilation of addons):

https://github.com/embroider-build/embroider/blob/ff31a8ad06953de43237d22b015aad79c2120961/packages/compat/src/v1-addon.ts#L352

and here (for stage3 compilation of everything):

https://github.com/embroider-build/embroider/blob/ff31a8ad06953de43237d22b015aad79c2120961/packages/compat/src/v1-app.ts#L253

The only place where I know that some things still undergo module-to-amd compilation before reaching webpack is in certain addons that have their own custom build pipelines that run babel explicitly. We still allow them to do that so they don't break.

Maybe you can debug further and see why these settings don't seem to be taking effect.

@bobisjan
Copy link
Contributor

bobisjan commented Mar 27, 2020

I'm confused, because I have a fresh Ember application created by Ember CLI v3.16.1, and the yarn list --pattern @embroider lists core and macros, but no compat.

➜  ember-date-fns git:(master) ✗ yarn list --pattern @embroider 
yarn list v1.22.4
├─ @embroider/core@0.4.3
└─ @embroider/macros@0.4.3
✨  Done in 0.92s.
➜  ember-date-fns git:(master) ✗ yarn why @embroider/compat
yarn why v1.22.4
[1/4] 🤔  Why do we have the module "@embroider/compat"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
error We couldn't find a match!
✨  Done in 0.85s.

I'm not sure how these settings can be applied without that package being installed?

EDIT

This is for using ember-auto-import, when I switch to embroider, then tree shaking is working as expected.

@ef4
Copy link
Collaborator

ef4 commented Mar 28, 2020

Sorry, I was the one confused, I mixed up my bug reports. I was thinking of embroider, not ember-auto-import. 😛

(The reason you have some embroider packages even in a standard app is that ember-auto-import already shares some embroider code.)

This is a good catch. I suspect this was a regression caused when we enabled babel transpilation of all auto-imported modules. It should be straightforward to fix.

The cleanest solution is to make sure that when we apply babel to auto-imported dependencies, we're only running @babel/preset-env, not the app's entire babel config.

@bobisjan
Copy link
Contributor

No worries, and thanks for the explanation 👍.

I have to apologise, because after more investigation made today, I have to take back my statement about ember-cli-babel and compileModules, turning off does not make it work.

The cleanest solution is to make sure that when we apply babel to auto-imported dependencies, we're only running @babel/preset-env, not the app's entire babel config.

I do think, that this is already happening and it is working as it should https://github.com/ef4/ember-auto-import/blob/master/packages/ember-auto-import/ts/webpack.ts#L128-L156.

I also do think now is that ember-auto-import works only around specifiers.

Given this application code.
// app/routes/application.js
import Route from '@ember/routing/route';
import { addDays } from 'date-fns';
Splitter knows about the `date-fns` specifier, but it does not take care of individual imported exports (named or default).
 {
  "app": {
    "static": [
      {
        "specifier": "date-fns",
        "entrypoint": "/Users/bobisjan/Developer/ember-date-fns/node_modules/date-fns/esm/index.js",
        "importedBy": [
          {
            "package": "ember-date-fns",
            "path": "ember-date-fns/routes/application.js",
            "isDynamic": false
          }
        ]
      }
    ],
    "dynamic": []
  },
  "tests": {
    "static": [],
    "dynamic": []
  }
}
This generates entrypoint like this, but when webpack sees `require` for `date-fns`, it does not know that only `addDays` from that file should be included, hence it includes everything.
if (typeof document !== 'undefined') {
  __webpack_public_path__ = (function(){
    var scripts = document.querySelectorAll('script');
    return scripts[scripts.length - 1].src.replace(/\/[^/]*$/, '/');
  })();
}

module.exports = (function(){
  var d = _eai_d;
  var r = _eai_r;
  window.emberAutoImportDynamic = function(specifier) {
    return r('_eai_dyn_' + specifier);
  };
    d('date-fns', [], function() { return require('/Users/bobisjan/Developer/ember-date-fns/node_modules/date-fns/esm/index.js'); });
})();

Does this make sense?

@ef4
Copy link
Collaborator

ef4 commented Apr 13, 2020

Does this make sense?

Yes, definitely. Thanks for investigating. That explains it.

We can fix this. The solution has two parts.

Part one is extending the splitter so it keeps track of all the names that are consumed by each import statement. That it relatively straightforward. It would add a new property to the ResolvedImport type.

Part two is a little more involved. Today, when we generate the webpack entrypoint we emit code like this:

d("some-library", [], function() { return require("./node_modules/some-library/dist/index.js") });

The code lives in what ember-auto-import calls its stagingDir. In the code, the d is really Ember's runtime AMD define (deliberately obscured so webpack won't try to handle it), and the require is seen by webpack as an AMD require. This is what marries together the two worlds of ember's loader and webpack's build.

For webpack to see the individual imports, we can't use AMD require, we need to use actual ES imports. But imports can't be synchronous and dynamic, and we don't want to eagerly evaluate every dependency. So instead, I think we need to introduce an extra level of modules. The code above would change to:

d("some-library", [], function() { return require("./some-library.js") });

And we would also emit some-library.js into the same stagingDir, containing:

export { only, the, used, names } from "./node_modules/some-library/dist/index.js";

@tomwayson
Copy link

Hi @ef4, I see that there are plans for ember-auto-import v2. Do you think this issue will be resolved in v2?

If not, is your proposed fix still valid and compatible w/ v2?

@ef4
Copy link
Collaborator

ef4 commented Apr 15, 2021

The proposed fix would still work. This hasn't been prioritized, but if anyone wants to work on it that would be good.

It's also non-breaking, so there's no pressure to squeeze it into 2.0.

@NullVoxPopuli
Copy link
Contributor

Hello everyone! thanks for participating in this issue report! 🎉

Just wanted to ask if this is still an issue with ember-auto-import 2.6.3?
If it is, does someone want to provide a reproduction repository?

thanks!!

@Windvis
Copy link

Windvis commented Jul 5, 2023

@NullVoxPopuli It's still an issue, yes. I've reproduced it in a Stackblitz: https://stackblitz.com/edit/github-bmw1gc

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

No branches or pull requests

7 participants