Skip to content

Initialize fileTimestamps and contextTimestamps in Compilation - #6883

Merged
sokra merged 4 commits into
webpack:masterfrom
mohsen1:remove-dead-code
Mar 29, 2018
Merged

sokra merged 4 commits into
webpack:masterfrom
mohsen1:remove-dead-code

Conversation

@mohsen1

@mohsen1 mohsen1 commented Mar 28, 2018

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?
refactor

Did you add tests for your changes?
no

If relevant, link to documentation update:

Summary
this.fileTimestamps && this.contextTimestamps is always falsy in Compilation. I looked into Tapable too and those don't exist there either. I see fileTimestamps in Compiler class. Is that related?

Does this PR introduce a breaking change?

Other information

Related to #6862

@ooflorent

Copy link
Copy Markdown
Contributor

It looks like that code was not dead 😕

@mohsen1

mohsen1 commented Mar 28, 2018

Copy link
Copy Markdown
Contributor Author

I can't follow what's going on here. How Compiler and Compilation classes are related?

@ooflorent

Copy link
Copy Markdown
Contributor

Compiler creates instances of Compilation:

https://github.com/webpack/webpack/blob/master/lib/Compiler.js#L428-L438

@sokra sokra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is definitely not dead, it's pretty important.

The timestamps are set by the Watching or by the CachePlugin.

Maybe initialize them in the constructor to undefined

@webpack-bot

Copy link
Copy Markdown
Contributor

@mohsen1 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@mohsen1 mohsen1 changed the title Remove dead code in Compilation.js Initialize fileTimestamps and contextTimestamps in Compilation Mar 28, 2018
@webpack-bot

Copy link
Copy Markdown
Contributor

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

@mohsen1 mohsen1 mentioned this pull request Mar 29, 2018
27 tasks done
@sokra
sokra merged commit d4f3c77 into webpack:master Mar 29, 2018
@sokra

sokra commented Mar 29, 2018

Copy link
Copy Markdown
Member

Thanks

mhegazy added a commit to mhegazy/webpack that referenced this pull request Apr 23, 2018
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.

4 participants