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

Lettable operators: scope hoisting prevents dead code removal #2981

Closed
jgoz opened this issue Oct 19, 2017 · 7 comments
Closed

Lettable operators: scope hoisting prevents dead code removal #2981

jgoz opened this issue Oct 19, 2017 · 7 comments

Comments

@jgoz
Copy link

jgoz commented Oct 19, 2017

RxJS version: 5.5.0

Code to reproduce:

webpack.config.js:

const rxPaths = require("rxjs/_esm5/path-mapping");
const webpack = require("webpack");
const path = require("path");
const UglifyJSPlugin = require("uglifyjs-webpack-plugin");

module.exports = {
    entry: "./entry.js",
    output: {
        path: path.resolve("./dist"),
        filename: "[name].bundle.js"
    },
    resolve: {
        alias: rxPaths("./node_modules")
    },
    plugins: [
        new webpack.optimize.ModuleConcatenationPlugin(), // enable scope hoisting
        new UglifyJSPlugin({
            parallel: true,
            uglifyOptions: {
                ecma: 5,
                compress: {
                    passes: 3
                }
            }
        })
    ]
};

entry.js:

import { map } from "rxjs/operators";

// do something with `map`

The above is a variation of the simple config in the lettable operators docs, modified to use webpack's new UglifyJS plugin.

When you use this config with code that imports from rxjs/operators, it will result in every operator being included in the final bundle whether or not it's being used. But if you comment out the ModuleConcatenationPlugin line, the unused operators will be properly removed from the output bundle.

This is happening because the ModuleConcatenationPlugin will move the re-exports in rxjs/operators/index.js into the same scope as the actual operator definitions, which prevents webpack from identifying whether those re-exports are actually being used.

Bundle output snippet with scope hoisting disabled:

/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0__audit__ = __webpack_require__(40);
/* unused harmony reexport audit */
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__auditTime__ = __webpack_require__(75);
/* unused harmony reexport auditTime */

Bundle output snippet with scope hoisting enabled:

/* unused concated harmony import audit */
/* concated harmony reexport */__webpack_require__.d(__webpack_exports__, false, function() { return audit; });
/* unused concated harmony import auditTime */
/* concated harmony reexport */__webpack_require__.d(__webpack_exports__, false, function() { return auditTime; });

Note the difference in unused harmony reexport audit vs concated harmony reexport; it's this difference that prevents UglifyJS from removing the unused operators in the latter case.

Recommendation:

This is not an rxjs issue, but I thought it might be useful to document the behaviour here and suggest removing ModuleConcatenationPlugin from the example webpack config. Or maybe acknowledging that scope hoisting implies a tradeoff of larger bundle size (when importing from rxjs/operators) vs. smaller parsing/resolving time.

@simonbuchan
Copy link

Thanks for speccing this, I ran into it just yesterday 😢 (but with the babel-minify plugin).
My workaround was to rewrite all imports: import { foo } from 'rxjs/operators/foo';, but that was no fun.

@jgoz
Copy link
Author

jgoz commented Nov 15, 2017

@simonbuchan Yeah, I played around with this for a long time and determined that even without scope hoisting, webpack+Uglify has a hard time with re-exports. It's fairly pessimistic and assumes that every import might have side effects, keeping them in the final bundle even if the actual exported objects are stripped. Definitely not an rxjs-specific issue.

To get around this, I ended up using babel-plugin-transform-imports to transform import { foo } from "rxjs/operators" into import { foo } from "rxjs/_esm5/operators/foo" with the following config:

{
  "rxjs/operators": {
    transform: "rxjs/_esm5/operators/${member}",
    preventFullImport: true,
    skipDefaultConversion: true
  }
}

This assumes a one-to-one match between the export name and the module name for these operators, so it's kind of brittle, but the reduction in bundle size was pretty significant.

@ptitjes
Copy link

ptitjes commented Nov 16, 2017

@jgoz Would you mind sharing your webpack config that uses babel-plugin-transform-imports please ?

@jgoz
Copy link
Author

jgoz commented Nov 16, 2017

const rxPaths = require("rxjs/_esm5/path-mapping");
const webpack = require("webpack");
const path = require("path");
const UglifyJSPlugin = require("uglifyjs-webpack-plugin");

module.exports = {
  entry: ["./entry.js"],
  output: {
    path: path.resolve("./dist"),
    filename: "[name].bundle.js"
  },
  devtool: "sourcemap",
  resolve: {
    alias: rxPaths(path.resolve(__dirname, "../../node_modules") /* might be unnecesary for you */)
  },
  module: {
    rules: [
      {
        test: /\.js$/,
        use: {
          loader: "babel-loader",
          options: {
            presets: [],
            plugins: [
              [
                require("babel-plugin-transform-imports"),
                {
                  "rxjs/operators": {
                    transform: "rxjs/_esm5/operators/${member}",
                    preventFullImport: true,
                    skipDefaultConversion: true
                  }
                }
              ]
            ],
            sourceMaps: true
          }
        }
      }
    ]
  },
  plugins: [
    new webpack.optimize.ModuleConcatenationPlugin(),
    new UglifyJSPlugin({
      parallel: true,
      sourceMap: true,
      uglifyOptions: {
        ecma: 5,
        mangle: true,
        compress: {
          passes: 3,
          sequences: false
        }
      }
    })
  ]
};

@ptitjes
Copy link

ptitjes commented Nov 17, 2017

Thank you @jgoz for the help. I succeeded to use a similar strategy as yours in my Ionic 3/Angular 5 + TypeScript application.

However I found I have even better results by doing the following:

  • use babel-plugin-transform-imports to do the import rewrites but simply with
{
  "rxjs/operators": {
    "transform": function (importName) {
      return 'rxjs/operators/' + importName;
    },
    preventFullImport: true,
    skipDefaultConversion: true
  }
}
  • then letting webpack do the binding to rxjs/_esm5:
resolve: {
  ...
  alias: rxPaths()
}

Indeed, it seems I have some rxjs code in my dependencies and the late binding to rxjs/_esm5 seems to also apply to those.

@dairyisscary
Copy link

I followed the great example of @jgoz and @ptitjes, but in my particular use case, I had trouble; I would get errors where webpack would not be able to resolve the transformed operator, even though it was present in the resolve alias.

I think its because import { mapTo } from 'rxjs/operators'; (transformed to import { mapTo } from 'rxjs/operators/mapTo'; matched both rxjs/operators and rxjs/operators/mapTo. I just did something like this to make it be an exact match (see webpack resolve documentation):

const rxPathsFactory = require("rxjs/_esm5/path-mapping");

const rxPaths = Object
  .entries(rxPathsFactory())
  .reduce((accum, [ rxImport, realPath ]) => {
    // We need these to be excat matches, so use the dollar at the end.
    return Object.assign(accum, { [`${rxImport}$`]: realPath });
  }, {});

I'm not really sure why this is not the default/what comes out of the path mapping itself.

@benlesh
Copy link
Member

benlesh commented Oct 2, 2019

I don't believe this is an issue any longer with newer bundlers. Closing for now.

@benlesh benlesh closed this as completed Oct 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants