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

Module request specifier for a non JS file does not pick up matching definition files #49970

Closed
tomrav opened this issue Jul 20, 2022 · 8 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@tomrav
Copy link

tomrav commented Jul 20, 2022

Bug Report

.d.ts files are not picked up for files that do not contain a .js extension in the module request specifier.

This happens when using the following configuration:

  • "moduleResolution": "node16" in tsconfig.json
  • "type": "module" in package.json

🔎 Search Terms

definition files, non js, third party

🕗 Version & Regression Information

This reproduces on version 4.7.4, but I think this issue exists since TypeScript began supporting "type": "module" configuration.

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about definition files

⏯ Playground Link

No playground as this reproduction requires multiple files.

💻 Code

Minimal reproduction repo - https://github.com/barak007/type-module-ts-bug

🙁 Actual behavior

The component.ts file, imports the component.st.css file, but the component.st.css.d.ts file is not picked up, and the import is not resolved.

🙂 Expected behavior

Pick up the module definition for the .st.css file and allow using it.

@tomrav tomrav changed the title Module request specifier for a non JS file do not pick up matching definition files Module request specifier for a non JS file does not pick up matching definition files Jul 21, 2022
@blayet
Copy link

blayet commented Jul 25, 2022

@barak007
Copy link

I work with @tomrav, and it's possible we weren't clear enough about our intent in our original post, I hope to make it clearer here.

There are a few things we can note:

  1. In esm the evaluation of the request specifier is host-specified (node, browser, etc...)
  2. We are talking about the node16 resolution (host) mode combined with a package.json in a type module configuration
  3. In this resolution mode, "relative import paths need full extensions", which we provide, in our case .css
  4. In node, you can use esm loaders to allow loading any non-JS file extension, converting them to es modules
  5. In node, without a custom loader, if you write an import with a specifier to a .css file the result is a failure with the message ERR_UNKNOWN_FILE_EXTENSION. It's important to note that node did pick up the imported .css file and attempted to process it. This means that the file resolution is working fine (also in TypeScript). But, the import receives no typings because the .d.ts is not properly matched.

Furthermore, similar to how esm loaders work in node, bundlers or browsers can also be used to load non-JS files. One example for this is import asserts, another would be a bundler's loader configuration.

Our repository reproduction demonstrates a case where we have a .css file, and a d.ts file with the same name next to it.
We then import the .css file (with full extension specifier) and expect the d.ts file next to it to provide the typings for said file, but it does not.

This raises the question, how can we "type" this .css file?

Up until now using the Node resolution mode (cjs), this worked fine and the d.ts files were picked up.
Both of the following imports picked up the component.st.css.d.ts file.

// resolution: Node
import * as T1 from "./component.st.css";    // works
import * as T2 from "./component.st.css.js"; // works

I think that the .d.ts file should be picked up for both the .css and .css.js requests in node16 resolution mode, and it would be true so long as there is actually a file that could be resolved by those requests.

Is this the source of the issue?

I think it boils down to the fact that .d.ts files for .js files are not using full extensions, the .js is replaced with the .ts at the end of .d.ts same for cjs and mjs.

/a.js ==> /a.d.ts

When looking at the .css example, what should be done?

/a.css      <-- original request in JS 

/a.css.d.ts <-- keeping the extension and adding the `.d.ts` extension
/a.d.css    <-- adding a `.d` before the `.css` extension
/a.d.ts     <-- dropping the extension and adding `.d.ts`, this is not possible

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jul 29, 2022
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.9.0 milestone Jul 29, 2022
@weswigham
Copy link
Member

Afaik this is pretty much as expected - for any extension that isn't a known js extension that maps to a known extension (eg, js to ts, mjs to mts, cjs to cts), as far as I know, we just append .d.ts to the path as a fallback mechanism. That can conflict with a js file, sure, but it's unlikely in practice (because how many people chain extensions amirite), and we've never done anything else. Maybe a .d.css type generic extension mechanism would make sense, but we wouldn't have a reasonably backwards compatible way to remove the .d.ts appending behavior, so it's not really worth adding a second extension to type the same input path.

@weswigham
Copy link
Member

Like, the .d.ts addition fallback behavior arises from trying a .js addition and falling back to equivalent ts extensions. Node esm doesn't have extension lookup, so we don't even try in an esm file on node16.

@barak007
Copy link

Not to go too deeply into implementation details, my logic is:

  1. in node I can write import { something } from "module.cpp";
  2. in node I can use a loader to compile the C++ code into a javascript module
  3. I expect that if I put a module.cpp.d.ts file next to the C++ file, that it will be used to provide typings
  4. I don't expect to add .js extension because I already provided full path with extension for the file

Same idea is true for the browser and bundlers.

I feel like matching definition files that sit next to a source file should not be related to the raw request or the resolution algorithm, but to the resolved file path:

  1. If the resolved file path ends with .cjs, .mjs, .js - Then replace the extension with .d.cts, .d.mts, .d.ts. I think that it should also be possible to just append .d.ts to the end of the filePath like: .js.d.ts, .cjs.d.ts, .mjs.d.ts.
  2. If the resolved file path ends with anything else - then just append .d.ts like: .cpp.d.ts

This way when the file x.cpp is imported with the request ./x.cpp, then it will match ./x.cpp.d.ts when imported using the .js extension as ./x.cpp.js will also matches x.cpp.d.ts

@weswigham
Copy link
Member

Hm, I mean, the issue with going with the "always lookup an appended .d.ts extension" route in esm is is means a ./file.css.d.ts on disk would then match both ./file.css and ./file.css.js specifiers, when only one of those is likely to exist at runtime, and the runtime isn't trying both locations to paper over the difference, like it is for a require in cjs.

@barak007
Copy link

barak007 commented Jul 30, 2022

That is why I suggested to also allow just appending .d.ts to match .js files. (or basically any file)

Consider the following project structure:

/app
   /index.js 
   /index.d.ts    <-- this will be picked up if there were no index.js.d.ts, currently it's not picked up
   /index.js.d.ts <-- this will be picked up because a full extension is a more complete match

   /index.css
   /index.css.d.ts

   /index.css.js 
   /index.css.js.d.ts

Structure with "conflict":

/app
   /index.js <-- import {} from "./index.css";
   /index.css
   /index.css.d.ts <-- this will be picked up because full extension match.
   /index.css.js.d.ts <-- this will be ignored because we already matched index.css.d.ts

When adding .js to the request

/app
   /index.js <-- import {} from "./index.css.js"; /* .js */
   /index.css
   /index.css.d.ts <-- this will be ignored because we already matched index.css.js.d.ts
   /index.css.js.d.ts <-- this will be picked up because full extension match.

There is a reason why you would want to import from ./index.css.js. if you build the css file to js in the dist the request will match the js file.

I think there is no ambiguity in this kind of matching, and the reason there is the possibility of conflicts in the first place is because the original matching was not for a full extension.

When emitting .d.ts files I would only generate them with full extension. index.ts --> index.js.d.ts

@andrewbranch
Copy link
Member

@weswigham closed by #51435?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

6 participants