use absolute path as key to store files, correctly handle scenarios w…

[vladima]

…hen file names differ only in casing. Fixes #5102, #5274, #3626, #4719, #2011

[mhegazy]

--useCaseSensitiveLookups or
--caseSensitiveFileNames

[mhegazy]

👍

[vladima]

calling @RyanCavanaugh and @DanielRosenwasser: any preferences/opinions regarding the name and description of the command line option?

[jbrantly]

There is some weird behavior coming about (I believe) due to these changes. In particular, getScriptSnapshot of the language service is now called with filePaths that are not absolute. I believe that's due to this change.

Is it expected that language service implementations now need to join on the currentDirectory?

[mhegazy]

@jbrantly can you share more details. that change was not intentional.

[jbrantly]

Sure. I caught this running the test suite of ts-loader on the nightly build. I have a test where I use the custom module resolution stuff (resolveModuleNames) to resolve a module at path components/myComponent. This winds up resolving to ~/common/components/myComponents (an absolute path). After this resolution is returned from resolveModuleNames a call to getScriptSnapshot is made with the path common/components/myComponents. Since I presume that all calls to getScriptSnapshot are absolute this messes things up.

As a secondary issue, after putting in a "fix" so that getScriptSnapshot still works by resolving to the current directory (path.resolve(process.cwd(), fileName)), updates to the file do not take effect. In other words, I can update that file's contents and version and call getEmitOutput on it but the old emit is returned instead.

Basically this:

file.version++;
file.text = contents;
console.log('before');
console.log(contents);
var output = langService.getEmitOutput(filePath);
console.log('after');
console.log(output.outputFiles[0].text);

results in:

before
export = 'newValue';
after
module.exports = 'oldValue';

If you want to duplicate yourself you can:

git clone https://github.com/TypeStrong/ts-loader.git
cd ts-loader
npm install
npm install [email protected]
npm run build
cd test/aliasResolution
webpack --watch

It will build fine initially, but if you were to throw a few console.logs into the loader source you'll see that things are not as they should be. And then if you update the dependent module the resulting bundle.js will not be correct because the language service didn't emit the updated source.

If you change to [email protected] things work as they should.

I can open new issues for these if you'd like.

[vladima]

I think I know what is going on. Will look into the solution tonight

[vladima]

the change was 'sort of' intentional and you've already pointed to the line with motivating comment:

// convert an absolute import path to path that is relative to current directory
// this was host still can locate it but files names in user output will be shorter (and thus look nicer).

this however means that implementations of CompilerHost and LanguageServiceHost must be able to handle all valid paths in both relative and non-relative forms in all methods that accept fileName. In your case this means that fileName = path.normalize(path.resolve(process.cwd(), fileName)); should be used not only in getScriptSnapshot but also in getScriptVersion. I can see that this part is non-obvious. Question to @mhegazy: maybe we should consider always pass absolute file names to methods of host?This in turn will mean that SourceFile.fileName will always be absolute path but we can shorten it when printing errors

[jbrantly]

@vladima Thanks for your feedback! I completely forgot about getScriptVersion so that's almost certainly the issue with updating files. IMO if you decide to not revert this behavior I think I would consider it a breaking API change since it's typical to use those paths as keys (ala here)

[vladima]

#5462 partially reverts this PR