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

Any file name with .d. in it ending with .ts is a declaration file in 5.0.2 even without --allowArbitraryExtensions #53319

Closed
dsherret opened this issue Mar 17, 2023 · 8 comments
Labels
Bug A bug in TypeScript Help Wanted You can do this

Comments

@dsherret
Copy link
Contributor

dsherret commented Mar 17, 2023

Bug Report

See the last "or" condition:

TypeScript/src/compiler/parser.ts

Lines 10178 to 10180 in a70c409

export function isDeclarationFileName(fileName: string): boolean {
return fileExtensionIsOneOf(fileName, supportedDeclarationExtensions) || (fileExtensionIs(fileName, Extension.Ts) && stringContains(getBaseFileName(fileName), ".d."));
}

Added as part of #51435 -- I think this was meant to be behind the --allowArbitraryExtensions flag?

> function checkAmbient(fileUrl) {
    const s = ts.createSourceFile(fileUrl, "");
    console.log((s.flags & ts.NodeFlags.Ambient) === ts.NodeFlags.Ambient)
}
undefined
> ts.version
'5.0.2'
> checkAmbient("file:///V:/test.d.ts$22-28.ts")
true
> checkAmbient("file:///V:/test.d.tsblah.ts")
true
> ts.version
'4.9.5'
> checkAmbient("file:///V:/test.d.ts$22-28.ts")
false
> checkAmbient("file:///V:/test.d.ts22-28.ts")
false

Right now the only way I see to set if a file type is ambient is through the extension. Maybe the API needs a better way for consumers to tell it "this is a declaration file"? Maybe on CreateSourceFileOptions? Then perhaps the code that does the arbitrary extension resolution could mark the declaration file that matches the arbitrary extension as ambient and the code here could be reverted.

🔎 Search Terms

extension declaration

🕗 Version & Regression Information

  • 5.0.2
  • I see this in main

🙁 Actual behavior

Declaration file specific errors

🙂 Expected behavior

No declaration file errors.

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this labels Mar 17, 2023
@RyanCavanaugh
Copy link
Member

I think the logic got a little twisted up here, it's intentional to not just look for exactly .d.ts since .d.cts and .d.mts are also valid declaration filenames. Just needs a tweak.

@dsherret
Copy link
Contributor Author

dsherret commented Mar 17, 2023

@RyanCavanaugh what needs to be tweaked here? I would help fix this, but I don't know what should change to fix it because maybe this is intended behaviour now.

To summarize the issue, the --allowArbitraryExtensions added support for things like .d.css.ts to be marked as declaration files. The issue is that this is done unconditionally even without --allowArbitraryExtensions when calling ts.createSourceFile.

I feel like a fix would be to have a way for the createSourceFile API to specify if something is a declaration file?

Right now the only way I see to set if a file type is ambient is through the extension. Maybe the API needs a better way for consumers to tell it "this is a declaration file"? Maybe on CreateSourceFileOptions? Then perhaps the code that does the arbitrary extension resolution could mark the declaration file that matches the arbitrary extension as ambient and the code here could be reverted.

Maybe an issue though is in the language server you probably want a way to tell if a file is a declaration file solely based on the file path I think? Maybe that could be figured out if a corresponding .ext file is there... so for example if file.ext is there then maybe file.d.ext.ts will be marked as a declaration file (seems maybe slightly unfortunate this is .d.ext.ts and not .ext.d.ts even with the reasoning in the blog post). Edit: or I guess it could just resolve .d.*.ts as a declaration file only when allowArbitraryExtensions is set in the tsconfig... I'm not sure at all though because I'm not that familiar with the ts language server

@RyanCavanaugh
Copy link
Member

@weswigham can you weigh in?

@weswigham
Copy link
Member

I think this was meant to be behind the --allowArbitraryExtensions flag?

Nope! These are always recognized as declaration files now; the flag only controls weather we issue an error on their use to resolve an import in .ts source files (which we know is technically a break, if very unlikely, but that's what the moral cover of 5.0 is for). They're also always valid from declaration file to declaration file, this way they can be used to describe things portably on DT. Generally, the less flags actually affect resolution, the better.

Edit: or I guess it could just resolve .d.*.ts as a declaration file only when allowArbitraryExtensions is set in the tsconfig... I'm not sure at all though because I'm not that familiar with the ts language server

WIldcard directory includes (like the compiler without any configuration) suck in all files ending in .ts, which happens to include all declaration files (since they also end in .ts), and then just filters out those declaration files whose original source files are also present.

@weswigham
Copy link
Member

I feel like a fix would be to have a way for the createSourceFile API to specify if something is a declaration file?

Nah, I don't think we'd like this. We like being able to map from declaration file path back to original source file path, and if you can mark arbitrary files as "declaration", we lose that capability.

@dsherret
Copy link
Contributor Author

That's fair. I think we can close this issue then?

@weswigham
Copy link
Member

Yeah, this is working as intended.

@dsherret
Copy link
Contributor Author

Btw, thanks for the clarification! I will just make a few small changes on our side and this is not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

3 participants