-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Do not lowercase typeReference directive name #58525
Conversation
@typescript-bot test this |
Starting jobs; this comment will be updated as builds start and complete.
|
Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
Hey @sheetalkamat, the results of running the DT tests are ready. Everything looks the same! |
@sheetalkamat Here are the results of running the user tests comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@sheetalkamat Here are the results of running the user tests comparing Everything looks good! |
@sheetalkamat Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@sheetalkamat Here are the results of running the top 400 repos comparing Everything looks good! |
@@ -117,7 +166,6 @@ Info seq [hh:mm:ss:mss] Files (4) | |||
Matched by default include pattern '**/*' | |||
../lib/@types/UpperCasePackage/index.d.ts | |||
Entry point for implicit type library 'UpperCasePackage' | |||
Type library referenced via 'UpperCasePackage' from file '../lib/@app/lib/index.d.ts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stops showing as this because of how this is processed. When the UpperCasePackage
typings are already found and are primary then they are not processed again and hence the file reason for this is never added. Thats a separate issue and am looking into updating that code as well but its not related to this change.
it just stopped showing because before this the package name by auto type ref was "UpperCasePackage" while by type Ref was "uppercasepackage" which made two distinct entries and hence processed again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems correct, but it also seems like a lot of people in the linked issue are misusing type reference directives. They’re not really supposed to resolve to files, right? They should be package directories in typeRoots
?
Yes they are but given that we dont disallow relative names in the We started doing this because of "Jquery.min" etc files and some other auto type Aquisition issues. But if you write "types" in your file, write it with correct casing is logical i think. |
As part of #9824 all type reference directives are lower cased so type="somePackage" results in looking for type reference of somepackage instead.
We also did not lowercase file names found as part of auto type references. So there was always some descripency with how we handled typeRef. Now with this change, it's as written.
I reverted that part and only kept the jsTypings to do the lowercasing to find the
@types
packages to install.This fixes issues with
forceConsistentCasingInFileNames
esp when using relative paths intypes
reference.#26948 becomes absolute with this
Fixes #45096