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

add pure modules #5435

Merged
merged 5 commits into from Sep 16, 2017

Conversation

Projects
None yet
10 participants
@sokra
Member

sokra commented Aug 3, 2017

What kind of change does this PR introduce?
feature

Did you add tests for your changes?
yes

If relevant, link to documentation update:
TODO

Summary
refactor harmony modules
support pure-module flag in package.json

Does this PR introduce a breaking change?
yes, internals changed

Other information

@clydin

This comment has been minimized.

Show comment
Hide comment
@clydin

clydin Aug 4, 2017

Would side-effect-free-module/sef-module/safe-module or some other variation be more appropriate? Being pure has a higher threshold than just having no side effects. Continuing and encouraging the redefinition of an existing well-defined term may not be the best path forward.

clydin commented Aug 4, 2017

Would side-effect-free-module/sef-module/safe-module or some other variation be more appropriate? Being pure has a higher threshold than just having no side effects. Continuing and encouraging the redefinition of an existing well-defined term may not be the best path forward.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Aug 4, 2017

Contributor

@sokra what do you think about changing the impl from looking at a package.json flag to instead detecting a pragma in the file?

Something like:

/**
 * @pure
 **/

Which would mark that file and all dependencies beneath as pure. Gives a little more flexibility for the module author to contain side effects to specific places while allowing the rest of the codebase to be properly treeshaken.

Contributor

probablyup commented Aug 4, 2017

@sokra what do you think about changing the impl from looking at a package.json flag to instead detecting a pragma in the file?

Something like:

/**
 * @pure
 **/

Which would mark that file and all dependencies beneath as pure. Gives a little more flexibility for the module author to contain side effects to specific places while allowing the rest of the codebase to be properly treeshaken.

@webpack-bot

This comment has been minimized.

Show comment
Hide comment
@webpack-bot

webpack-bot Aug 8, 2017

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

webpack-bot commented Aug 8, 2017

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra modified the milestone: webpack 4 Aug 9, 2017

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Aug 9, 2017

The current template generates __webpack_require__ calls with unquoted module ids: var core_es5 = __webpack_require__(/oeL); note the /oeL part, which should have been quoted.

bigger snippet:
/***/ "Lr3D":
/***/ (function(module, __webpack_exports__, __webpack_require__) {

"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/core/@angular/core.es5.js
var core_es5 = __webpack_require__(/oeL);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/platform-browser/@angular/platform-browser.es5.js
var platform_browser_es5 = __webpack_require__(fc+i);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/material/core/overlay/overlay.js + 1 modules
var overlay_overlay = __webpack_require__(kZVf);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/material/core/overlay/overlay-state.js
var overlay_state = __webpack_require__(7aoq);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/cdk/portal/portal.js + 1 modules
var portal = __webpack_require__(yPlj);

IgorMinar commented Aug 9, 2017

The current template generates __webpack_require__ calls with unquoted module ids: var core_es5 = __webpack_require__(/oeL); note the /oeL part, which should have been quoted.

bigger snippet:
/***/ "Lr3D":
/***/ (function(module, __webpack_exports__, __webpack_require__) {

"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/core/@angular/core.es5.js
var core_es5 = __webpack_require__(/oeL);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/platform-browser/@angular/platform-browser.es5.js
var platform_browser_es5 = __webpack_require__(fc+i);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/material/core/overlay/overlay.js + 1 modules
var overlay_overlay = __webpack_require__(kZVf);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/material/core/overlay/overlay-state.js
var overlay_state = __webpack_require__(7aoq);

// EXTERNAL MODULE: /angular/aio/node_modules/@angular/cdk/portal/portal.js + 1 modules
var portal = __webpack_require__(yPlj);
@danny-andrews

This comment has been minimized.

Show comment
Hide comment
@danny-andrews

danny-andrews Aug 10, 2017

@probablyup

what do you think about changing the impl from looking at a package.json flag to instead detecting a pragma in the file?
...
Gives a little more flexibility for the module author to contain side effects to specific places while allowing the rest of the codebase to be properly treeshaken.

This seems like a v2 feature, if at all. It's really bad practice to have side effects as part of an import that isn't of the form import 'my-lib'; (in which case, your entire module is impure). If your library has some side-effects it needs to perform, why not have that done in a method your library exposes?

danny-andrews commented Aug 10, 2017

@probablyup

what do you think about changing the impl from looking at a package.json flag to instead detecting a pragma in the file?
...
Gives a little more flexibility for the module author to contain side effects to specific places while allowing the rest of the codebase to be properly treeshaken.

This seems like a v2 feature, if at all. It's really bad practice to have side effects as part of an import that isn't of the form import 'my-lib'; (in which case, your entire module is impure). If your library has some side-effects it needs to perform, why not have that done in a method your library exposes?

@danny-andrews

This comment has been minimized.

Show comment
Hide comment
@danny-andrews

danny-andrews Aug 10, 2017

@clydin

Being pure has a higher threshold than just having no side effects. Continuing and encouraging the redefinition of an existing well-defined term may not be the best path forward.

Mind elaborating on what this "higher threshold" entails? The way I understand it, "pure" means:

  1. Output is determined only by inputs
  2. No side-effects

Number 1 seems to be met due to the nature of ES modules. If I do import { a } from 'lib' I will always get the same thing, no?

danny-andrews commented Aug 10, 2017

@clydin

Being pure has a higher threshold than just having no side effects. Continuing and encouraging the redefinition of an existing well-defined term may not be the best path forward.

Mind elaborating on what this "higher threshold" entails? The way I understand it, "pure" means:

  1. Output is determined only by inputs
  2. No side-effects

Number 1 seems to be met due to the nature of ES modules. If I do import { a } from 'lib' I will always get the same thing, no?

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Aug 10, 2017

Contributor

@danny-andrews it's not quite that cut and dry. Here's an example of some recent code:

We use styled-components at work and made a file that injects some global styles as soon as the module is interpreted. This is imported as part of a tree of layout components.

However, the file is also consumed separately as one of the files fed into a webpack entry array; that case can't be wrapped in a callable function as webpack doesn't have a way of saying "load this file as an entry and call X function."

Contributor

probablyup commented Aug 10, 2017

@danny-andrews it's not quite that cut and dry. Here's an example of some recent code:

We use styled-components at work and made a file that injects some global styles as soon as the module is interpreted. This is imported as part of a tree of layout components.

However, the file is also consumed separately as one of the files fed into a webpack entry array; that case can't be wrapped in a callable function as webpack doesn't have a way of saying "load this file as an entry and call X function."

@clydin

This comment has been minimized.

Show comment
Hide comment
@clydin

clydin Aug 10, 2017

@danny-andrews , The value of a may or may not always be the same. It depends on the definition of a, its dependencies, and any potential previously executed code. Javascript's flexibility makes being pure difficult.

Regardless, whether an import statement is actually pure is not really relevant. The optimizations presented here rely on the lack of side effects within the modules. The overall effect will be people marking there packages/modules/etc. as "pure" when what they are actually doing (as per the documentation and code) is saying that there are no side effects within the aforementioned; and not that anything is indeed pure (true or otherwise). As a result, "pure" now equates with "no side effects". This may seem like a distinction without a difference. However and especially when dealing with optimizations, there is a difference. Also of relevance is Uglifyjs which appears to bare a similar concern.

clydin commented Aug 10, 2017

@danny-andrews , The value of a may or may not always be the same. It depends on the definition of a, its dependencies, and any potential previously executed code. Javascript's flexibility makes being pure difficult.

Regardless, whether an import statement is actually pure is not really relevant. The optimizations presented here rely on the lack of side effects within the modules. The overall effect will be people marking there packages/modules/etc. as "pure" when what they are actually doing (as per the documentation and code) is saying that there are no side effects within the aforementioned; and not that anything is indeed pure (true or otherwise). As a result, "pure" now equates with "no side effects". This may seem like a distinction without a difference. However and especially when dealing with optimizations, there is a difference. Also of relevance is Uglifyjs which appears to bare a similar concern.

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra Aug 10, 2017

Member

The current template generates webpack_require calls with unquoted module ids: var core_es5 = webpack_require(/oeL); note the /oeL part, which should have been quoted.

rebased. Branch now includes the fix.

Member

sokra commented Aug 10, 2017

The current template generates webpack_require calls with unquoted module ids: var core_es5 = webpack_require(/oeL); note the /oeL part, which should have been quoted.

rebased. Branch now includes the fix.

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Aug 10, 2017

@sokra I'm at 9a3f259 and my relatively complex app is now building and seems to be fully functional.

Code splitting works as expected even for large modules like @angular/cdk and @angular/material.

This version also resolves the memory usage/leak issue I saw in the previous builds.

Lastly, it seems that module concat is working better now, which resulted in even smaller bundles.

Overall, this is the first build on this branch that seems to be working really well for our use-cases.

The only downside is initial build performance is slower because webpack has to crawl through many more files (previously we rolled-up small es modules into one big one, so it was faster for webpack to process it, but at that point we could no longer codesplit these large modules).

IgorMinar commented Aug 10, 2017

@sokra I'm at 9a3f259 and my relatively complex app is now building and seems to be fully functional.

Code splitting works as expected even for large modules like @angular/cdk and @angular/material.

This version also resolves the memory usage/leak issue I saw in the previous builds.

Lastly, it seems that module concat is working better now, which resulted in even smaller bundles.

Overall, this is the first build on this branch that seems to be working really well for our use-cases.

The only downside is initial build performance is slower because webpack has to crawl through many more files (previously we rolled-up small es modules into one big one, so it was faster for webpack to process it, but at that point we could no longer codesplit these large modules).

@Andarist Andarist referenced this pull request Aug 13, 2017

Closed

Migrate to ES6 modules #1968

@sokra sokra changed the base branch from master to next Sep 11, 2017

@umidbekkarimov umidbekkarimov referenced this pull request Sep 14, 2017

Closed

Tree Shaking? #2748

sokra and others added some commits Aug 8, 2017

Refactor harmony modules
separate side effects from specifier
fixes TDZ for export let/const
bigger bundles are caused by correct export const/let behavior

BREAKING CHANGE: Internal have changed. Plugins could rely on them.
rewrite README.md a bit to make it clearer
mostly copyediting for grammar and tone

@sokra sokra changed the title from [WIP] add pure modules to add pure modules Sep 14, 2017

sokra added some commits Sep 14, 2017

Merge branch 'next' into feature/pure-module
# Conflicts:
#	test/statsCases/commons-plugin-issue-4980/expected.txt
#	test/statsCases/scope-hoisting-multi/expected.txt
#	test/statsCases/tree-shaking/expected.txt

@sokra sokra merged commit c47bdc6 into next Sep 16, 2017

5 of 9 checks passed

codecov/changes 1 file has unexpected coverage changes not visible in diff.
Details
codecov/patch 91.23% of diff hit (target 94.82%)
Details
codecov/project 94.62% (-0.21%) compared to 9e5765f
Details
coverage/coveralls Coverage decreased (-0.2%) to 94.755%
Details
ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@sokra sokra deleted the feature/pure-module branch Sep 16, 2017

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Sep 16, 2017

Just saw that the change from pure-module -> side-effects didn't make its way into the examples README.

tilgovi commented Sep 16, 2017

Just saw that the change from pure-module -> side-effects didn't make its way into the examples README.

@Andarist Andarist referenced this pull request Sep 16, 2017

Closed

rollup + uglify #1

@umidbekkarimov umidbekkarimov referenced this pull request Oct 4, 2017

Closed

Simplify unit test environment #1111

2 of 3 tasks complete

nikku added a commit to bpmn-io/min-dash that referenced this pull request Oct 20, 2017

chore(project): indicate this is a side-effect free library
Indicate that this library is side-effect free to bundles (read:
webpack).

Cf. webpack/webpack#5435
@andrewbranch

This comment has been minimized.

Show comment
Hide comment
@andrewbranch

andrewbranch Nov 13, 2017

This is fantastic! Thanks for the great work!

Part 1: Feature request

Along the lines of @probablyup’s comment, it would be really nice to be able to do this within large projects, not just at the package.json level.

We have a large monorepo with many entry points. Within that repo we have a library of common React components that are used throughout the project. For convenience, we currently re-export almost every component from an index barrel. This necessarily causes every component to be bundled together, even though some entry points only use a small subset of them.

If we were able to mark that index barrel or the components that are re-exported as side-effect-free, we’d be able to leverage CommonsChunkPlugin to split that bundle of components up more strategically.

Part 2: Advice request

If such a feature will not be implemented (or will not be implemented soon), is there a way to trick Webpack into thinking that internal collection of components is its own module? Perhaps adding a package.json within the project and setting up an alias for it?

andrewbranch commented Nov 13, 2017

This is fantastic! Thanks for the great work!

Part 1: Feature request

Along the lines of @probablyup’s comment, it would be really nice to be able to do this within large projects, not just at the package.json level.

We have a large monorepo with many entry points. Within that repo we have a library of common React components that are used throughout the project. For convenience, we currently re-export almost every component from an index barrel. This necessarily causes every component to be bundled together, even though some entry points only use a small subset of them.

If we were able to mark that index barrel or the components that are re-exported as side-effect-free, we’d be able to leverage CommonsChunkPlugin to split that bundle of components up more strategically.

Part 2: Advice request

If such a feature will not be implemented (or will not be implemented soon), is there a way to trick Webpack into thinking that internal collection of components is its own module? Perhaps adding a package.json within the project and setting up an alias for it?

@danny-andrews

This comment has been minimized.

Show comment
Hide comment
@danny-andrews

danny-andrews commented Nov 14, 2017

Whooo!

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