Fix incorrect relative module name detection#18576
Conversation
| if (path.charCodeAt(1) === 58) { | ||
| if (path.charCodeAt(2) === 47) | ||
| return 3; | ||
| return 2; |
There was a problem hiding this comment.
please undo changes to files under lib
| "compilerOptions": { | ||
| /* Basic Options */ | ||
| "target": "es5", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', or 'ESNEXT'. */ | ||
| "module": "commonjs", /* Specify module code generation: 'none', commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */ |
There was a problem hiding this comment.
not sure why yo had to change these.
There was a problem hiding this comment.
These fixes are made by gulp baseline-accept.
Completely unrelated to the issue, but I suppose it would not hurt to fix a typo in the comments.
Should I revert it?
There was a problem hiding this comment.
looks like we did not catch these failures. that should be fixed by #18583
There was a problem hiding this comment.
Great! I've removed these changes from the PR.
| } | ||
| if (path.charCodeAt(1) === CharacterCodes.colon) { | ||
| if (path.charCodeAt(2) === CharacterCodes.slash) return 3; | ||
| return 2; |
There was a problem hiding this comment.
Should it still return 2 if path.length === 2 to handle input like C:?
There was a problem hiding this comment.
seems like a good suggestion.
we should add some unit tests for this function, but that can be a different change.
There was a problem hiding this comment.
C: does not look like a valid path to me, so I'd rather say no.
Maybe I'm just missing some use cases.
There was a problem hiding this comment.
c: is a valid folder name on windows. so we should allow it.
There was a problem hiding this comment.
Well, I've made a bit of a research.
c:is NOT a valid folder name on Windows.:is not a valid character for a folder name.- In command line
cd c:does nothing,dir c:displays the content of a current directory. So it seems likec:is not a path, but rather a volume label, which is not handled by Windows the same way as a regular path. - Node's
path.isAbsolutePathreturnstruefor bothc:/andc:\\, butfalseforc:. - Node's
path.resolvereturns current directory forc:.
Based on this behaviour a cannot see how c: is a valid path.
Can you provide some code to illustrate your point?
There was a problem hiding this comment.
fair enough. can you update the test baselines
There was a problem hiding this comment.
I did, and something went wrong : [
I've run exactly
jake
jake runtests-parallel
jake baseline-accept
Now there are tons of changed and new files in the PR.
What should I've done differently?
1036ec8 to
9b188bd
Compare
9b188bd to
924945f
Compare
|
Any news on this PR? |
|
Superseded by #19702 |
Fixes #18536