Repository navigation
Add missing class member initializations found by TypeScript - #6910
Conversation
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
is that Codacy/PR Quality Review something I should look into? |
|
ping @sokra |
| this.ids = null; | ||
| this.debugId = debugId++; | ||
| this.name = name; | ||
| this.removedModules = undefined; |
There was a problem hiding this comment.
This is not correct. A Chunk doesn't have this probably.
You probably found this in the HMR code. The template takes a Chunk | { id, modules, removedModules }. This is a kind of fake Chunk with this extra info.
| super(options); | ||
| this.range = range; | ||
| this.valueRange = valueRange; | ||
| this.critical = undefined; |
There was a problem hiding this comment.
Can be removes as it's inherited from ContextDependency
| class ChunkModuleIdRangePlugin { | ||
| constructor(options) { | ||
| this.options = options; | ||
| this.chunks = undefined; |
There was a problem hiding this comment.
This is incorrect.
Actually the usage of this.chunks below is incorrect. It should be compilation.chunks instead.
This happend probably while the migration from function to arrow function.
Better do a separate PR here, as we should also add tests for this.
There was a problem hiding this comment.
Funny. I was doing a PR (#6954) while your comment popped 😅
|
@mohsen1 since you have a bunch of these smaller PR's do you have any opposition to anyone taking them if they have small corrections? Happy to find some folks to help out here! |
|
@TheLarkInn I love to get help for this. Having all those initializations in one PR is not ideal. I like to have a pull request per class so we can discuss what's going on and why those initializations are necessary. |
|
That's perfect. Then we can enlist help from the masses knowing this :).
…On Mon, Apr 9, 2018, 12:57 PM Mohsen Azimi ***@***.***> wrote:
@TheLarkInn <https://github.com/TheLarkInn> I love to get help for this.
Having all those initializations in one PR is not ideal. I like to have a
pull request per class so we can discuss what's going on and why those
initializations are necessary.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6910 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADQBMJRl-YRm6pXDXLQeAivqqTaCcCpBks5tm70cgaJpZM4TBK-R>
.
|
|
Thanks |
6 participants
What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
no
If relevant, link to documentation update:
n/a
Summary
As part of #6862 I'm adding class member initializers for properties that were added to class instances
Does this PR introduce a breaking change?
Other information