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

refactor(ES6): upgrade Compilation to ES6 #3767

Merged
merged 6 commits into from Jan 5, 2017

Conversation

ahmedelgabri
Copy link
Contributor

What kind of change does this PR introduce?

ES5 => ES6 refactor

Did you add tests for your changes?

all tests are passing

Summary

Refactoring to ES2015/6

Does this PR introduce a breaking change?

No

- constructor
- templatesPlugin
- addModule
- getModule
- findModule
- buildModule
- proccessModuleDependencies
- addModuleDependencies
- _addModuleChain
- addEntry
- prefetch
- rebuildModule
- finish
- unseal
- seal
- sortModules
- reportDependencyWarnings
- addChunk
- assignIndex
- assignDeep
- proccessDependenciesBlockForChunk
- removeChunkFromDependencies
- applyModuleIds
- applyChunkIds
- sortItesBeforeIds
- sortItemsWithModuleIds
- sortItemsWithChunkIds
- summarizeDependencies
- createHash
- modifyHash
- createModuleAssets
- getPath
- getStats
- createChildCompiler
- checkConstraints
@TheLarkInn
Copy link
Member

Impressive work. Well done.

@TheLarkInn TheLarkInn merged commit 86169bd into webpack:master Jan 5, 2017
@TheLarkInn
Copy link
Member

Thank you so much. This class was not an easy refactor. 🙇 👏 🍾

@ahmedelgabri
Copy link
Contributor Author

Thank you so much. This class was not an easy refactor. 🙇 👏 🍾

It's just too long 😄

@ahmedelgabri ahmedelgabri deleted the refactor-Compilation branch January 5, 2017 18:27
@ahomatthew
Copy link

ahomatthew commented Sep 28, 2017

@ahmedelgabri I think you unintentionally introduced a bug where if you are missing a file that is imported _this is prematurely being set to null before the error callback happens.

Here -> https://github.com/webpack/webpack/blob/master/lib/Compilation.js#L222 _this is set to null by the time it fires, and therefore triggers quite the red herring. Line where it's being set to null -> https://github.com/webpack/webpack/blob/master/lib/Compilation.js#L363

For background, i'm running a yarn build with create-react-app's react-scripts build. I'm doing an import on a relative path to a json file in my project.

What should happen:

sally@sunshine:/path/to/code$ yarn build
yarn run v1.1.0
$ yarn run build-css && react-scripts build
Rendering Complete, saving .css file...
Wrote CSS to /path/to/code/src/index.css
Wrote 1 CSS files to /path/to/code/src/
Creating an optimized production build...
Failed to compile.

Module not found: Error: Can't resolve './config.json' in '/path/to/code/src'

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

What's actually happening:

sally@sunshine:/path/to/code$ yarn build
yarn run v1.1.0
$ yarn run build-css && react-scripts build
Rendering Complete, saving .css file...
Wrote CSS to /path/to/code/src/index.css
Wrote 1 CSS files to /path/to/code/src/
Creating an optimized production build...
/path/to/code/node_modules/webpack/lib/Compilation.js:219
				_this.errors.push(err);
				     ^

TypeError: Cannot read property 'errors' of null
    at errorAndCallback (/path/to/code/node_modules/webpack/lib/Compilation.js:219:10)
    at errorOrWarningAndCallback (/path/to/code/node_modules/webpack/lib/Compilation.js:251:14)
...

@ahmedelgabri
Copy link
Contributor Author

@ahomatthew I'm not sure if it was me or is it related to something else because when I look at the file before my changes It's exactly the same after I converted it:

_this = null

_this = null;

_this.errors.push(err);

_this.errors.push(err);

@ahomatthew
Copy link

Ah got it, I'll file a bug.

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

Successfully merging this pull request may close these issues.

None yet

3 participants