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

Simplify extension priorities and move dts to lowest priority #34713

Closed

Conversation

weswigham
Copy link
Member

Fixes #33623

Where there are js (or json) files side-by-side with a declaration file of the same name, we now only load the .js file via wildcard. The declaration file can still be included via other means, but will no longer be implicitly included when allowJs is on. (Consequently, now if you have allowJs and declaration on and are using a wildcard matcher, repeated invocations of the compiler should now find the same set of inputs - at least via wildcard lookup.)

Technically speaking, I also removed all the extension priority groups to simplify extension priority handling - we do not appear to have any test cases this affects, but I do wonder if ranking .ts higher than .tsx is observable - in theory, it's not, since inputs are lexicographically ordered, so we'd always find the .ts before the .tsx before, anyway, so the extension priorities only had the affect of grouping declaration files with .js and .jsx files (which we now no longer wish to do).

@weswigham
Copy link
Member Author

cc @RyanCavanaugh who knows the origin story for the extension priority system and can maybe drop some knowledge as to if it still has good reason to be once .d.ts ranks lower than everything else.

@sandersn sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020
@weswigham
Copy link
Member Author

reping @RyanCavanaugh again I guess

@sandersn
Copy link
Member

sandersn commented Mar 4, 2020

@sheetalkamat probably has opinions too.

@sandersn
Copy link
Member

sandersn commented Mar 9, 2020

This is one of the hilarious PRs that doesn't load the reviewers list so I assigned both @sheetalkamat and @RyanCavanaugh instead of putting both in the reviewers list.

@@ -5781,13 +5781,13 @@ namespace ts {
* List of supported extensions in order of file resolution precedence.
*/
export const supportedTSExtensions: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Dts];
export const supportedTSExtensionsWithJson: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Dts, Extension.Json];
export const supportedTSExtensionsWithJson: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Json, Extension.Dts];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't ideal I think...

Consider scenario::

// moduleA.ts
import { x } from "./moduleB";

// moduleB.js
export const x = 10;

When you build first time files in your program with allowJs and emitDeclarationsOnly will be:
moduleA.ts, moduleB.js
Next time they will be
moduleA.ts, moduleB.d.ts and moduleB.js since module resolution prefers up d.ts over .js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what we do today. This PR fixes that, such that the .d.ts is never preferred if the js is present, assuming allowJs is on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is rootFiles will be moduleA.ts and moduleB.js but final list of files will contain .d.ts since that's what module resolution will resolve to? https://github.com/microsoft/TypeScript/blob/master/src/compiler/moduleNameResolver.ts#L1096

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yeah, the order we check extensions in module resolution likely needs to match the order wildcards use. I'll look at changing it to use the same source, ideally.

@RyanCavanaugh
Copy link
Member

The root cause of .d.ts being lowest priority is that .d.ts can be a build output, and we always want to prefer build inputs to build outputs since otherwise we'd get "stuck" in a state where the developer would have to clean their outputs to get a correct fresh build. The interaction with .json in play now is obviously very subtle and we need to come up with a coherent story about what that means.

I'm pretty nervous about breaking someone's odd build setup (because let's be honest the test coverage in this area is less than great) when the associated issue here doesn't really have customer reports yet. Simplification is good but we can put this on the shelf for the time being and come back to it when we have a more concrete idea of what scenarios are being addressed.

@weswigham
Copy link
Member Author

weswigham commented Mar 10, 2020

The interaction with .json in play now is obviously very subtle and we need to come up with a coherent story about what that means.

Not just json - jsx? with allowJs as well. (Which is actually more of a problem, imo)

@sheetalkamat
Copy link
Member

Module resolution, wild card matching are mingled.. when resolving module you want to have .d.ts at higher priority than .js because those have better info especially if those files are from some external source.. say node_modules is simplest case but people have weird module resolution setups where those might not be in node_modules So in my opinion this makes it more complicated and unless we have cleared up rules on when to prefer which extension this is going to be complicated.. I agree with @RyanCavanaugh that this needs to be addressed when we have sufficient reports of the issue and have concrete plan and matrix of scenarios of expectations.

@sandersn
Copy link
Member

sandersn commented Apr 1, 2020

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

@sandersn sandersn closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Investigate altering extension priorities for wildcard loading
5 participants