Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lowercase filename in triple slash directives #45096

Closed
gaurav5430 opened this issue Jul 19, 2021 · 18 comments · Fixed by #58525
Closed

lowercase filename in triple slash directives #45096

gaurav5430 opened this issue Jul 19, 2021 · 18 comments · Fixed by #58525
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@gaurav5430
Copy link

gaurav5430 commented Jul 19, 2021

Bug Report

Referencing a file name in triple slash directives automatically looks for the lower case filename

🔎 Search Terms

  • triple slash directive
  • lowercase filename

🕗 Version & Regression Information

have seen it since ts > 3.x

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about triple slash directives / lowercase filename

⏯ Playground Link

not sure how to reproduce it in the playground
This requires multiple files, and there is no easy way to verify

💻 Code

// fileOne.d.ts
...
...
// file2.d.ts
// <reference types="../fileOne.d.ts">
...
...

🙁 Actual behavior

on case sensitive OS, i got a warning from tscompiler about the casing being incorrect.

🙂 Expected behavior

should not get this warning

More Info:
I guess this is related to #26948
and it seems this is correct behaviour.

If that is the case, it would be great to document it alongside triple slash directives or add it to FAQs
https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html

@DanielRosenwasser
Copy link
Member

Do you have forceConsistentCasingInFileNames on?

@gaurav5430
Copy link
Author

Yes, it is on

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 20, 2021

Sorry, I misread the issue - I don't know if that's actually relevant, though it does help avoid the issue.

If the underlying file is fileone.d.ts, on a case-sensitive system, this behavior is actually expected. Is that correct? The original post doesn't seem to give a full repro.

@gaurav5430
Copy link
Author

yes, my point is more about the fact that it seems the triple slash directives by default convert all filenames to lowercase, but that is not documented anywhere

@BPapp-MS
Copy link

This bug broke a couple pipelines for us the other day- it seems that compilation on an OS which is case insensitive produces reference tags in d.ts files which are lowercase, whereas on an OS which is case-sensitive it maintains proper casing in the output. Failures were caused when we started validating the reference directives in the generated API document, and there was platform-based mismatch. It seems unnecessary to lowercase the path on case-insensitive platforms in the first place.

@BPapp-MS
Copy link

Example source:

// src/source.tsx
/// < reference path="./ReferencedTypes.d.ts" />

Compiled declaration file in Unix (case sensitive):

// lib-esm/source.d.ts
/// < reference types="./ReferencedTypes.d.ts" />

Generated declaration file in Windows (case insensitive):

// lib-esm/source.d.ts
/// < reference types="./referencedtypes.d.ts" />

@RyanCavanaugh RyanCavanaugh added the Needs More Info The issue still hasn't been fully clarified label Jul 20, 2021
@RyanCavanaugh
Copy link
Member

/// < reference types="./ReferencedTypes.d.ts" />

I'm not sure how you got into a state where a relative path directive got updated to a types directive (I'm sure it's possible but don't know how to reconstruct that). Can you share a complete repro?

@fatcerberus
Copy link

yes, my point is more about the fact that it seems the triple slash directives by default convert all filenames to lowercase

You said the underlying file is lowercase. I don't think it's a blind conversion like you suggest--it's observing that the filename in the directive doesn't match what's on disk and warning you about that. Blindly converting all filenames to lowercase wouldn't make sense and would actually break things on a case-sensitive FS.

@gaurav5430
Copy link
Author

yes, my point is more about the fact that it seems the triple slash directives by default convert all filenames to lowercase

You said the underlying file is lowercase. I don't think it's a blind conversion like you suggest--it's observing that the filename in the directive doesn't match what's on disk and warning you about that. Blindly converting all filenames to lowercase wouldn't make sense and would actually break things on a case-sensitive FS.

Hey, not really, that's not what I meant.

the underlying file name is fileOne.d.ts created on Mac OS
and on Mac OS it works alright if I have a
<reference path="./fileOne.d.ts" />

but it gives an error on Linux about inconsistent casing

@Uzlopak
Copy link

Uzlopak commented Mar 2, 2022

We got today a Bug Report on mongoose by @Davis-owen regarding this bug. This is totally undocumented and unexpected behavior.

I think the bug was introduced with #10340 by @andy-ms when the lowercasing was only about the npm package name.

I think that the solution is to check if it is a string, which does not contain a slash "/" and lowercase it, and if it contains a slash to split the string and only lowercase the the first part and keep the rest unchanged. Or maybe even remove the lowercasing. It was imho in the first place wrong, to implicitly transform the reference path to lowercase.

@Uzlopak
Copy link

Uzlopak commented Mar 2, 2022

Here again a proof, that the lowercase is because of that PR which was only targetting to lowercase the npm package name.

s-panferov/awesome-typescript-loader#326 (comment)
by @DanielFallon

As of that merge, all reference tags are used as lowercase'd

@Uzlopak
Copy link

Uzlopak commented Mar 2, 2022

Btw. this is not about blaming @andy-ms . I just wanted to tag @andy-ms so that he maybe adds his thoughts on this. But now I see he is not active on github anymore.

Well anyway: Is removing the lowercasing actually an option? Unfortunately Typescript Codebase seems to be kind of heavy to work in, so I would not be able to provide a PR with the necessary Quality.

Or we just make it crystal clear in the documentation that filenames of types should be lowercase.

@sheetalkamat
Copy link
Member

This had come up in #26948

@sheetalkamat
Copy link
Member

This is not about d. Ts emit but how we lowercase type ref names for file

@jakebailey
Copy link
Member

Sorry, I thought this was about emit 😅

@pbryant-xag
Copy link

pbryant-xag commented Apr 30, 2024

+1 on Linux, affecting https://www.npmjs.com/package/stripe

Feature request raised there since this issue hasn't had any love for a long while.

@rhyeen
Copy link

rhyeen commented May 1, 2024

+1. Issue seems resolved on MacOS for typescript@4, but not on Linux.

@xavdid-stripe
Copy link

I'd be happy to send a PR over to TS if this is something the team is willing to approve! While we could downcase the entire stripe-node project, we'd prefer not to. It does seem like an unintended consequence on the TS side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.