-
Notifications
You must be signed in to change notification settings - Fork 12
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
some fixes: #1
some fixes: #1
Conversation
- simplify hex-to-decimal conversion - really stable, reproducible module ID calculation - don't fix module ID collisions by choosing a different ID, because that makes the ID unstable; instead, throw an exception - provide a "seed" option so that collisions can be fixed manually
A note on the This PR may cause builds to fail when two modules get the same module ID. This is by design because such collisions are problematic for the solution itself. In such a situation, a To solve the problem, a different The seed is also included in the file names, so everything remains unique. |
@jampy Thanks so much for your PR. I agree with your point that the old Your solution is awesome, but it seems like just extremely reduce happenstance of Once we try to make module id (hash) shorter, it absolutely causes Let's think about it. Awesome PR really! |
👍 to this change. Making the hash a bit longer makes hash collisions very unlikely. Excited to try out this module! Wish something like this was enabled by default in Webpack. |
@zhenyong thanks
I'm not sure I understood your question, sorry. The basic principle is still yours. As with any hash, collisions are basically always possible. The larger the hash, the less probable it is. The hash computed by this PR is 28 bits long (2^28 possible IDs). I choose 28 bits because I guess that in JavaScript at least a 32 bit signed integer can be considered safe. Perhaps also 63 bits would be possible - could be worth to check out. The PR could be improved to use up to 31 bits - it just takes a little more effort (a little bit more math than just extracting a substring). The size of the hash (in my version) really only affects the probability of a collision (and the probability of a failed build). The IDs should however remain stable in any case. BTW, I'm using this in a large project (~1500 modules) and it works flawlessly so far :-) |
@KyleAMathews |
@jampy I hope the original codes not make harm to ur large project O(∩_∩)O~ I agree with that your solution can kill almost 99.9%(maybe ^_^) collision. In a sense, it is the equivalent of changing Here is anthor issue for both current code and your PR , that manually change "seed" to kill collision is not elegant for CI . Really a puzzle ha ^_^! @KyleAMathews Let me think about more. |
Actually I think the probability of a collision is somewhat like 1 / 268435456 * (number of modules). With 1500 modules that's 0,0005%. With a 31 bit hash that would be 0,00007%.
A hash can never be 100% collision-safe. You can just reduce the probability.
That's true, but I doubt there can be a solution that's hash based avoiding this problem completely. However, I think that it's worth it and the build should already fail on the development machine, even before reaching CI. |
Not really. Your original For example: The crucial part is really that Just think of this situation (with your version):
So, you have two different IDs - great. Now "module A" is removed from the codebase. This causes "module B" to get the ID 4950515253545556 - which is the ID that module A had. So, the main purpose of the plugin is lost. Do you see the problem now? Do you see any problems with my PR or would you mind simply accepting it? Thanks. |
@jampy I really know the original problem from begining, thanks your detail explanation. |
awesome! thanks! |
This PR fixes some issues that I think the original code has.
First of all, I think that the old
genModuleId()
is dangerous because it tries to "fix" module ID collisions by pratically choosing a different ID. However, this makes a module ID dependent to the other modules in the build - effectively defeating the purpose of the stable ID.