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
Fix module id conflict on a case insensitive OS #3317
Conversation
96871e0
to
7162ab6
Compare
I think this commit also fixes #3139 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would need to check but I am not sure you can do it that way as existingNames
is also the bundle object in some cases. Changing the names here could have far reaching consequences and could also be considered a breaking change as suddenly, all names on the bundle object would be lowercase. My guess is that some part of the code assumes that a chunk is on the bundle object under its original name. Not yet sure about a better approach, though.
7162ab6
to
4abdd4e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3317 +/- ##
==========================================
+ Coverage 93.17% 93.18% +<.01%
==========================================
Files 172 172
Lines 6041 6042 +1
Branches 1801 1801
==========================================
+ Hits 5629 5630 +1
Misses 221 221
Partials 191 191
Continue to review full report at Codecov.
|
@lukastaegert I submit another approach which won't change names in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment. Also it is mandatory for this fix to move forward to have tests that demonstrate the problem that is solved and also how potential edge-cases are handled if there are relevant ones.
For this feature, a test from the chunking-form category would be easy to set up.
src/utils/renderNamePattern.ts
Outdated
name = name.substr(0, name.length - ext.length); | ||
let uniqueName, | ||
uniqueIndex = 1; | ||
while (existingNames[(uniqueName = name + ++uniqueIndex + ext)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are files Date.js
and Date1.js
, a file named date.js
would be renamed date1.js
and thus conflict with the other name because you are not being case-insensitive here. For a quick fix, instead of iterating over the existing names you could create a Set a-priori to reference against:
const existingNamesLowercase = new Set(Object.keys(existingNames).map(key => key.toLowerCase()));
if (!existingNamesLowercase.has(name) return name;
...
while (existingNamesLowercase.has((uniqueName = name + ++uniqueIndex + ext).toLowerCase());
4422141
to
1a4b1f0
Compare
Suppose we have to files named `date.js` and `Date.js`, `makeUnique` treats these two files are different currently. But on a case insensitive OS, they are the same file. The commit try to fix the problem.
1a4b1f0
to
290de04
Compare
@lukastaegert Updated and test included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Suppose we have to files named
date.js
andDate.js
,makeUnique
treats these two files are different currently. But on a case insensitive OS, they are the same file. This commit try to fix theproblem.