stripe / stripe-node

Node.js library for the Stripe API.
https://stripe.com
MIT License
3.9k stars 755 forks source link

stripe-node bumps into TypeScript issue 45096 for triple slash directives when forceConsistentCasingInFileNames #2076

Closed pbryant-xag closed 4 months ago

pbryant-xag commented 7 months ago

Is your feature request related to a problem? Please describe.

Logging this as a FR because the issue is caused by an long-open TS issue, and other packages have worked around this issue by lower-casing all directory and filenames.

On a case-insensitive filesystem (such as macOS in default config), this package produces both cased (title-case) filenames and triple slash reference types= directives that are cased. So far so good.

However, TS has an open issue that results in a TypeScript build-time error if tsconfig.json > compilerOptions.forceConsistentCasingInFileNames==true (default is true, for good reason).

That means that users of this package who wish to tsc (build) on macOS are forced to set forceConsistentCasingInFileNames to false.

I know this is an uncommon configuration, and I'm not certain is this is critical or even relevant, but our working example involves:

(I don't have time just now to repro this in a more plain vanilla config sorry).

The tsc build error message is: "File name {git repo dependency's node_modules/stripe directory and file} differs from already included file name '{primary project node_modules/stripe directory and file}' only in casing"

"Type library referenced via 'stripe/types/WebhookEndpoints' from file {git repo dependency's node_modules/stripe directory and file} with packageId 'stripe/types/webhookendpoints.d.ts@14.25.0'" <-- notice that the filename is lower-cased.

In both of those filesystem locations, the directory and file names are cased (title case).

So tsc is deciding to look for a lower-cased filename even through all references to the file in all locations in the npm install filesystem are cased (title case), and all directories and filenames themselves are also cased, and the /// <reference types="" /> triple slash directive is also cased.

As above, the work-around is to forceConsistentCasingInFileNames to false, but you shouldn't be forced to do that.

The issue does not occur when building on Ubuntu, for example (with forceConsistentCasingInFileNames to set to either true or false).

Describe the solution you'd like

In other packages (eg Automattic/mongoose) they have lower-cased their filenames to side-step this issue. Perhaps that might be an option here.

Describe alternatives you've considered

No response

Additional context

No response

pbryant-xag commented 7 months ago

Actually it is way worse than described above. On Linux, the project doesn't build at all, with forceConsistentCasingInFileNames set to false or true. It simply fails to find the relevant files on the filesystem because it is looking for lowercase file names when they are actually cased (title-case) filenames.

So in short, when you have triple slash reference types directives, you can't tsc build on Linux.

rhyeen commented 7 months ago

Yup! Exact same issue. Using typescript@4 seems to remove this issue for me for MacOS, but not in Linux.

helenye-stripe commented 7 months ago

Can you provide more details on what exactly is causing this failure in Linux builds so I can reproduce it? Does it only occur with nested dependency on Stripe as well as having a dependency in the current project? @pbryant-xag

pbryant-xag commented 7 months ago

The only working model of the behaviour that we have currently is indeed with a nested git dependency on Stripe as well as a dependency in the main project. Without forceConsistentCasingInFileNames set to false, when it comes time for the main project's direct dependency on the Stripe package to be installed (if I have the ordering correct) then tsc complains that it is trying to install lower-cased versions of the Stripe directories and files, but there are existing versions of the same directories and files in title-case. If you set forceConsistentCasingInFileNames to false then tsc stops complaining about that, and on a case-insensitive filesystem like macOS there is no further problem.

However, the main and bigger issue is a variation on the above, which is that regardless of the forceConsistentCasingInFileNames setting, on Linux (case-sens fs, and our deployment environment OS) you get a similar but different error message at build time that the triple slash reference type file locations can't be found. tsc is looking for the lower-case version of those files whereas only the title-case versions exist on the filesystem, and to Linux these are of course just not the same files, and so are not found. The triple slash locations are correctly in title-case, but tsc is lower-casing those before looking for them (as is made clear in the TS GH issue linked above). Ahead of any actual testing and proving, for me this error is not directly related to there being both a nested dependency and a direct dependency on the stripe package. The root cause is that there is a triple slash reference type with a (correctly) case-sensitive file location which isn't found by tsc at build time because it (incorrectly) lower-cases them. The fact that there is an auto-created triple slash directive of this type is probably because the Stripe package is a dependency of a git dependency. Obviously if the main project had manually-created triple slash reference type statements to title-cased files, that would cause the problem also. Those triple slash statements are auto-generated in our case (presumably by npm install) and we have no direct control over whether they are created, so as long as we are using types from the Stripes package, we get those triple slash statements.

In our case, or only available work-around was to stop using the Stripe types in our nested dependency, which means we're using TypeScript but still can't be type safe with the Stripe NPM package :(

So I would suggest manually creating the triple slash reference type directives in a single project, referring to title-case filenames, and then you should see the problem at build time on Linux.

HTH

rhyeen commented 6 months ago

@helenye-stripe I don't think the "as well as having a dependency in the current project" is part of the issue. I think it's merely that if Stripe is a nested dependency. Here is my project structure that is causing the issue:

root-project
--> our-library
       --> stripe

Where our-library builds just fine, but root-project fails with the reported error. I've sanity checked and root-project does not import anything from stripe. I also do not export stripe directly from our-library. It is only that our-library has a dependency on stripe that is enough to cause root-project to fail. Obviously, some parts of our-library import stripe and in turn parts of root-project import those our-library exports, but not direct dependency.

I have confirmed this by reviewing the package-lock to ensure no other dependencies depend on stripe and have done a clean install just to be sure.

xavdid-stripe commented 6 months ago

Hey y'all! Some good news here. Looks like this was fixed in https://github.com/microsoft/TypeScript/pull/58525! It's available in the nightly build and will go out stable in the next release.

Can you test your setup against the nightly and verify that everything works as expected? If so I think we can close out this issue on our end.

rhyeen commented 5 months ago

So far so good. We'll be doing a full release in the next couple of weeks, but our dev CI/CD is now passing again with the Typescript change. I'll keep you posted once we're fully confident that we're out of the woods.

xavdid-stripe commented 4 months ago

We're going to close as fixed, but please let us know if there are still issues!