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

Fixed false collision exception #6

Merged
merged 1 commit into from
Nov 18, 2016
Merged

Fixed false collision exception #6

merged 1 commit into from
Nov 18, 2016

Conversation

x2es
Copy link
Contributor

@x2es x2es commented Nov 14, 2016

for some reason webpack may call plugin several times for the same module - this is not collision

I propose to cache modulePath and skip exception if the same module appeared again.

@jgable
Copy link

jgable commented Nov 15, 2016

I am also seeing multiple of the same modules come through my webpack pipeline. Also, the multiples have an empty string for module.request which makes their module.libIdent return a weird value.

I've actually fixed this by grabbing the sourcePath from module._source._name but that seems kinda sketchy.

@zhenyong
Copy link
Owner

@x2es Thanks.

This solution works for most case, but can not fix the problem like #2 .

@jampy
Copy link

jampy commented Nov 16, 2016

@zhenyong

I don't think it's opposed to fix #2, but rather addresses a completely different issue.

The proposed solution looks flawless to me and IMHO there is no possibility for a regression. It rather seems like an important enhancement.

(that means that if you agree on the added value of this PR and even if it is not related to #2 you most probably will want to accept it - unless you see a concrete problem with it).

@zhenyong
Copy link
Owner

@jampy Thanks remind.

@x2es Since This branch has conflicts that must be resolved, do you mind I merge it via cli ? Or send a new PR ?

@zhenyong zhenyong merged commit f87f2f7 into zhenyong:master Nov 18, 2016
@zhenyong
Copy link
Owner

Merged

@x2es
Copy link
Contributor Author

x2es commented Nov 18, 2016

Thank you for merge!

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