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

Bug: Priority of imports .json is higher ranked as .ts #47870

Closed
frank-dspeed opened this issue Feb 13, 2022 · 13 comments
Closed

Bug: Priority of imports .json is higher ranked as .ts #47870

frank-dspeed opened this issue Feb 13, 2022 · 13 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@frank-dspeed
Copy link

+1

If I turn on resolveJsonModule and put a MatchmakingConfig.ts and a MatchmakingConfig.json in the same path, a nasty unexpected behavior awaits:

// this results in MatchmakingConfig.json being imported, not MatchmakingConfig.ts!
import MatchmakingConfig from "./path/to/MatchmakingConfig";

Allowing .ts suffix will let us specify .ts files specifically.

Originally posted by @tonygiang in #37582 (comment)

Solution

Priority needs to get changed anyway .ts needs to get resolved with a higher priority then .json

@timreichen
Copy link

ref: #37582

@RyanCavanaugh
Copy link
Member

@weswigham any thoughts?

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Feb 17, 2022
@weswigham
Copy link
Member

OP, for now, you can just use the .js extension in your import, it'll refer to the TS source; you don't need ts extension imports.
But I'm actually surprised that .json is a higher priority extension to try than ts source - that seems... Off? Moreover, I can't reproduce that locally? When I write imports they correctly prefer the .ts file to the .json one. Could you give a more complete repro?

@RyanCavanaugh RyanCavanaugh added Needs More Info The issue still hasn't been fully clarified and removed Needs Investigation This issue needs a team member to investigate its status. labels Feb 18, 2022
@frank-dspeed
Copy link
Author

Originally posted by @tonygiang in #37582 (comment) can you please publish a reproducer repo anything that we can run that shows that failure?

@tonygiang
Copy link

Yes, here it is: ts-sandbox.zip

And here is a screenshot showing the direct contradiction between VS Code's suggestion icon and the actual output:
2022-02-19 00_37_17-index ts - ts-sandbox - Visual Studio Code

Tested on Node 16.14.0 LTS.

@dnalborczyk
Copy link

dnalborczyk commented Feb 18, 2022

this tripped me off a bit, I thought the node.js extension-less file resolution was preferring json over js.

it seems the bug is actually in ts-node. when you compile the repro with npx tsc, you can run the transpiled code with node index.js and you'll see type: "Module"

now, if you run ts-node after the transpilation again, it'll correctly output type: Module as well. it's possible it runs the .js script as opposed to the .ts script the second time around. (haven't looked at the docs if this is intended)

@weswigham
Copy link
Member

weswigham commented Feb 18, 2022

Yeah... That's a ts-node issue, not us, from what I can see. We resolve to the .js (or .ts) file first. I'm not even sure if ts-node fully supports module: nodenext yet.

@frank-dspeed
Copy link
Author

frank-dspeed commented Feb 19, 2022

@weswigham i know they are still working on it TypeStrong/ts-node#1361

and other issues i spot them from time to time in my projects at rollup plugin typescript we did also not handle all edge cases.

i my self try to algin the typescript functionality with the rollup one so that we maybe do not need to both implement the resolve algos maybe when typescript did implement proper resolve algos we could drop plugin node-resolve

@tonygiang
Copy link

tonygiang commented Feb 19, 2022

I see the real issue now. Times like this makes me kind of want TypeScript to ship with a standard JIT engine so that issues don't get fragmented to different stacks. Deno runtime has a similar issue also stemming from running untranspiled .ts files directly. Instead of importing the .json file, Deno just finds nothing and throws a "Module not found" error. This could have been 1 issue on 1 standard runtime instead of 2.

There was apparently an Import Assertions proposal submitted to TC39 that is at Stage 3 at the time of writing which resolves this issue for good with an additional assert { type: "javascript" } or assert { type: "json" } statement. So now I'm not sure what direction the TypeScript project is going to resolve this issue now. Implement this proposal ahead of time and as a side benefit, create some polyfill for older JS engines, perhaps?

@dnalborczyk
Copy link

dnalborczyk commented Feb 19, 2022

I see the real issue now. Times like this makes me kind of want TypeScript to ship with a standard JIT engine so that issues don't get fragmented to different stacks.

not sure what that would solve. typescript is providing the type-checker and transpiler, ts-node is taking that and hooks it into the require hook to transpile on-the-fly. I assume the bug stems from leaving the node.js resolve algorithm in order (.js -> .json -> .node(?)) and picks up the .ts extension afterwards. just a bad guess.

There was apparently an Import Assertions proposal submitted to TC39 that is at Stage 3 at the time of writing which resolves this issue for good with an additional assert { type: "javascript" } or assert { type: "json" } statement. So now I'm not sure what direction the TypeScript project is going to resolve this issue now.?

https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#import-assertions

keep in mind typescript is not down-leveling this syntax to a commonjs equivalent, although I'm thinking it could (or even should, at least when targeting commonjs/node.js), meaning you can use it only with a module target.

node.js has support now as well, and I'm assuming deno will too:

https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#import-assertions

@tonygiang
Copy link

It has to do with tooling consistency. Currently, we have VS Code and TypeScript both developed by the same company with the expectation that .ts files are transpiled before execution. ts-node and Deno were created with the expectation to run untranspiled .ts files directly and created 2 issues on 2 different 3rd-party projects and produced behaviors inconsistent with what will be suggested by VS Code.

If a JIT engine was developed by the same company too, the best case scenario is that the developers walk into either VS Code or the JIT Engine with the other tool in mind and align expectation so that this would never become an issue. Maybe the JIT Engine will force a pre-transpile if it detects a .json file with the same name with a .ts file, or maybe VS Code will simply give you a warning in that case and now you can rename some files and get no more runtime surprise. The worst case scenario is that it became 1 issue instead of 2.

@frank-dspeed
Copy link
Author

@tonygiang there is also ongoing work on typed json as far as i am aware of your jit arrgument is valid and the conclusion that the other projects are breaking behavior is also correct.

i think still that using rollup as loader would solve the issues at present i can workaround any issues like that via my rollup + plugin typescript setup but i was not able to isolate that into a useable generic product at the current point in time as there are many moving parts.

i do not remember what deno did use to run code but it was a relativ simple zero configuration bundler that they use to run the code. So deno has no own code to run code they are creating bundlers it was parcel
so anything that is valid for parcel is valid for deno at the end deno works with v8 so the only supported module system is ESM with importMaps and absolute or relativ pathes.

@frank-dspeed
Copy link
Author

lets close this as it is not a typescript bug. but in the moduleResolution documentation on typescriptlang should maybe be a example in what order json gets loaded when it gets loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

6 participants