Skip to content

Use a Map for dependencyTemplatesHashMap instead of a WeakMap - #6922

Merged
sokra merged 3 commits into
webpack:masterfrom
mohsen1:map-not-weakmap
Apr 4, 2018
Merged

Use a Map for dependencyTemplatesHashMap instead of a WeakMap#6922
sokra merged 3 commits into
webpack:masterfrom
mohsen1:map-not-weakmap

Conversation

@mohsen1

@mohsen1 mohsen1 commented Apr 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?
Refactor (potential bug fix)

Did you add tests for your changes?
No

If relevant, link to documentation update:
N/A

Summary
WeakMap can not have string keys. In getHashDigest following statement indicates keys of this map is string:

dependencyTemplatesHashMap.get("hash");

Setting a string as key of a WeakMap throws an error:

> const wm = new WeakMap()
undefined
> wm.set('foo', global)
TypeError: Invalid value used as weak map key
    at WeakMap.set (native)
>

Using primitives as key for WeakMap doesn't have any benefits because GC doesn't work with primitives
Does this PR introduce a breaking change?
Probably not... Hopefully not!

Other information

Found by TypeScript compiler. Part of #6862

@webpack-bot

webpack-bot commented Apr 1, 2018

Copy link
Copy Markdown
Contributor

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

@vkurchatkin

vkurchatkin commented Apr 1, 2018

Copy link
Copy Markdown

It looks like this code doesn't work anyway:

  • dependencyTemplatesHashMap.set is never called
  • dependencyTemplatesHashMap.get is only called with hash key
  • dependencyTemplates argument in getHashDigest is not used

@mohsen1

mohsen1 commented Apr 2, 2018

Copy link
Copy Markdown
Contributor Author

getHashDigest is being called here but you're right, I don't see nowhere dependencyTemplatesHashMap getting any values set. With this information getHashDigest is always returning a sting ending with -undefined.

@sokra

sokra commented Apr 2, 2018

Copy link
Copy Markdown
Member

dependencyTemplatesHashMap.get("hash") should be dependencyTemplates.get("hash") instead. The WeakMap can be removed.

@sokra
sokra merged commit f1993f4 into webpack:master Apr 4, 2018
@sokra

sokra commented Apr 4, 2018

Copy link
Copy Markdown
Member

Thanks

@sokra

sokra commented Apr 4, 2018

Copy link
Copy Markdown
Member

I'm still wondering why this didn't cause any issue...

@mohsen1 mohsen1 mentioned this pull request Apr 8, 2018
Merged
27 tasks done
@renovate renovate Bot mentioned this pull request Apr 17, 2018
Closed
mhegazy added a commit to mhegazy/webpack that referenced this pull request Apr 23, 2018
@mhegazy
Add note for webpack#6922
2e3f234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sokra sokra sokra approved these changes
+1 more reviewer
@ooflorent ooflorent ooflorent approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mohsen1 @webpack-bot @vkurchatkin @sokra @ooflorent