Fix module id conflict on a case insensitive OS #3317
Conversation
yesmeck
force-pushed
the
fix-module-name
branch
from
96871e0
to
7162ab6
Compare
|
I think this commit also fixes #3139 |
There was a problem hiding this comment.
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.
yesmeck
force-pushed
the
fix-module-name
branch
from
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.
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.
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());
yesmeck
force-pushed
the
fix-module-name
branch
3 times, most recently
from
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.
yesmeck
force-pushed
the
fix-module-name
branch
from
1a4b1f0
to
290de04
Compare
|
@lukastaegert Updated and test included. |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Suppose we have to files named
date.jsandDate.js,makeUniquetreats these two files are different currently. But on a case insensitive OS, they are the same file. This commit try to fix theproblem.