fix(typescript): normalize emittedFiles to prevent unrecoginized token

[glenn2223]

Rollup Plugin Name: typescript

This PR contains:

• bugfix

Are tests included?

• yes (bugfixes and features will not be merged without tests)

NOTE: Though I don't know the source of the issue - in order to force/replicate it - so the test succeeds pre and post change

Breaking Changes?

• no

List any relevant issue numbers: #892 #1083 #1238

Description

The id passed to async load(id) has certain edge cases when drives letters can de different cases or filenames can have its case changed. By normalising the emittedFiles we will always find the code we need and, as such, will not throw

Proof of issue/concept

Machine1: Windows 10.0.19044

I was able to compile TopMarksDevelopment/JavaScript.Autocomplete, but not TopMarksDevelopment/JavaScript.HoverBox. The only difference was that the index file was named Index instead of index. Changed that and then it compiled.

Rather than trudging on - because I was to improve open source, when I can 😁 - I started tinkering. When debugging I noticed that the code wasn't being returned as there was this name inconsistency between that in emittedFiles and the id received in load(id).

Applied the change replaced the compiled code in my project's node_modules and it compiled, whilst keeping the file name as Index.

Machine 2: Windows Server 2019 10.0.17763

I have a server that was having the same issue - this time attempting with gulp.

Debugging this time showed me that the drive letter character case was being changed for some reason - and not on every file that it was working on at the time.

Again, replaced the source with the changed compile code and it once again compiled. YAY!

[shellscape]

I'd like to get some clarity about what causes this edge case - why emittedFiles contains asset names that pass; is it TypeScript? is it this plugin? Also looks like these changes broke some existing tests on Windows.

[glenn2223]

I'd like to get some clarity about what causes this edge case - why emittedFiles contains asset names that pass; is it TypeScript? is it this plugin?

The name that gets passed to the load(id) function is wrong - the emittedFiles[] is right - as shown further down

This means that when the getEmittedFile(...) is called the filename isn't found and no code is returned and, as such, rollup thinks there's no parser installed

So, strictly speaking, it's a rollup issue. But, personally, I think normalizing should transform the case too, when comparing, to stop any edge cases, wherever they may be/pop-up

I'm unable to replicate/locate the issue with the windows server, but it was randomly getting a lowercase drive letter for some files - which again would mean find doesn't work

Also looks like these changes broke some existing tests on Windows.

Sorry, I'll take a look at the tests

[glenn2223]

Also looks like these changes broke some existing tests on Windows

Looks like the tsconfig.tsbuildinfo files are being dumped inside a cache folder .rollup.cache - which isn't accounted for in the tests

[glenn2223]

Not sure why the typescript test failed, worked in my run 🤷‍♂️. Plus completely unrelated to my changes 😁

[shellscape]

So, strictly speaking, it's a rollup issue

This was my suspicion based on the original description of the problem. I'd like to get this addressed in Rollup core before we make concessions for that in plugins.

[lukastaegert]

So, strictly speaking, it's a rollup issue

I do not think this is the case. Rollup core does not change imported ids. It does not change slashes and it definitely does not do anything with drive letters. Between emission and load hook, the id is passed through all resolveId hooks that all have the full power to do that. I strongly suspect TypeScript/the TypeScript plugin rewrote at least the slashes in the resolveId hook of the TypeScript plugin.

I remember having some older issue around TypeScript normalizing slashes while Rollup does not.

[glenn2223]

I do not think this is the case. Rollup core does not change imported ids. It does not change slashes and it definitely does not do anything with drive letters. Between emission and load hook, the id is passed through all resolveId hooks that all have the full power to do that. I strongly suspect TypeScript/the TypeScript plugin rewrote at least the slashes in the resolveId hook of the TypeScript plugin.

I remember having some older issue around TypeScript normalizing slashes while Rollup does not.

Thanks for the reply. Sorry, I'm not too familiar with the rollup structure/architecture.

So I've found the actual cause of the issue. In my config, I have input: 'src/index.ts' which is the cause of the naming inconsistency.

I tested with src/iNdex.ts in the config and resolveId(imported, ...) matched the config - not the actual filename. As the resolveId(..., importer) is null this typescript plugin returns null so that rollup (or I suppose another plugin) can resolve it. After the first return, the same function is called again, but this time with the KIND OF resolved path "C:\\Users\\Glenn\\Documents\\GitHub\\HoverBox\\src\\iNdex.ts" (note the name actually hasn't been resolved to match the actual file. In my opinion the actual path should be resolved correctly - as rollup already knows the file exists.

---

In regards to the drive letter changing randomly. There are some inconsistencies in npm's path resolutions - like nodejs/node#37307. Again, in my opinion, comparisons of any form should be case normalized too.

[glenn2223]

Note: with the change I get the below

[glenn2223]

So case sensitivity is ignored by node (yes, I know there's more to it). Below is an article touching on the (not-so)random case changes in paths and drive letters:
https://mnaoumov.wordpress.com/2019/10/18/windows-case-insensitive-file-names-nightmares-with-node-js/

Basically, I think it's now at the point of doing any includes type checks with the case normalised too. Which maybe should be mentioned to all plugins, noted on the plugin template and FAQ'd too 🤷‍♂️.

I'll take a look and see if there are any other includes in the typescript plugin. Maybe it's worth having a function/class in the utility module that can handle case-insensitive checks - I suppose I'd need both (in the case of this plugin), a function for arrays and something for key-value pairs

[glenn2223]

I'll take a look and see if there are any other includes in the typescript plugin

Don't think there is any, checked the cache version (right after my change) and that does file checks rather than string comparisons - so I left it alone.

[glenn2223]

Hey @shellscape / @lukastaegert, sorry to pester but have you any thoughts/progress on this? Would you like me to create a PR for some insensitive comparison utility tool(s)? Would you like two types - as I mentioned above (copied below)?

Maybe it's worth having a function/class in the utility module that can handle case-insensitive checks - I suppose I'd need both (in the case of this plugin), a function for arrays and something for key-value pairs

[shellscape]

I read through the Node core issue and have read all of the info you've dropped here. Thanks for putting all of this together.

So case sensitivity is ignored by node

Yes. It's a legacy issue that they really should have fixed by now, but the rate of error is still considered an edge case. They could sneak a fix into a major since the release schedule is so much more frequent now, but not sure why they don't.

This is precisely why tools like ESLint have plugins like eslint-plugin-import to assert that files exists at the location one is trying to require or import from. This of course, doesn't catch things like setting the entry file for a Rollup config, however.

So I've found the actual cause of the issue. In my config, I have input: 'src/index.ts' which is the cause of the naming inconsistency.

This is really the crux; user error. (should not be read as an insult)

---

My thoughts are that this isn't really something that I'm keen on merging, because I think that case sensitivity is a good thing in the majority of cases. Paths are only normalized based on separator (

plugins/packages/pluginutils/src/normalizePath.ts

Line 5 in 9b231e7

const normalizePath: NormalizePath = function normalizePath(filename: string) {

) but not casing.

That said, this might also be a cause of woe in the scenario for this PR

plugins/packages/typescript/src/diagnostics/host.ts

Lines 36 to 38 in 1a4f2bd

	getCanonicalFileName: (fileName) =>
	ts.sys.useCaseSensitiveFileNames ? fileName : fileName.toLowerCase()
	};

[glenn2223]

Sorry for the delay. Not a problem @shellscape, thanks for considering & no insult taken 😉 - we are all custom to user error (ESPECIALLY MINE!)

I needed it for a typescript extension I've been working on but since the issue has gone 🤷‍♂️