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

feature: Requesting support for node16/nodenext module resolution #1441

Closed
kf6kjg opened this issue Apr 5, 2023 · 14 comments
Closed

feature: Requesting support for node16/nodenext module resolution #1441

kf6kjg opened this issue Apr 5, 2023 · 14 comments
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved

Comments

@kf6kjg
Copy link

kf6kjg commented Apr 5, 2023

EDIT: Fixed in #1400 - thanks @carlocorradini!

Description

When using this library in a project that has the TSConfig setting moduleResolution set to node16 the TS compiler throws errors on every import and export ... from call.

Example:

../../node_modules/type-graphql/dist/index.d.ts(1,15): error TS2834: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Consider adding an extension to the import path.

Proposed solution

  1. Change the tsconfig.json#/compilerOptions/moduleResolution setting to node16.
  2. Bulk alter the codebase to correct the import and export paths to have the full path to the target file with the extension.
  3. Manually fix the outliers in the code: converting implicit index.js into explicit being one such.

This should have no deleterious effect since, as far as I am aware, no version of TS or Node cared about having the filename and extension until ES6 came along.

@carlocorradini
Copy link
Contributor

This feature is already available in PR #1400

We have to wait till it is merged and published :)

@kf6kjg
Copy link
Author

kf6kjg commented Apr 5, 2023

I'd read through that PR to see if it had this before posting, but not checked the code. So am I correct you are resolving this via using the @/ paths option so that you can use absolute paths instead of relative ones, thus bypassing the error without switching to the node16 module resolution strategy?

@carlocorradini
Copy link
Contributor

Nope.
The code supports both CJS and ESM.
Moreover, all absolute paths are transformed to relative.
You can check it by executing:

npm run build

@carlocorradini
Copy link
Contributor

If something is not clear let me know and I'll try to explain it better 🥳

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

Ok, so I checked out the maintainability branch from your repo @carlocorradini. I executed npm ci && npm run build and inspected the compiled code in the build folder. What I found is that the result still causes the error that thsi ticket was opened to address.

For example, the import in build/typings/schema/build-context.d.ts given below will trigger the error TS2834 mentioned in the OP.

import { IOCContainer } from "../utils/container";

If instead that compiled to the following it would not cause the error.

import { IOCContainer } from "../utils/container.js";

@carlocorradini
Copy link
Contributor

We can have .js in declarations too.
Wait 5 minutes. I'll push a fix and then you can try ok

@carlocorradini
Copy link
Contributor

@kf6kjg Try now

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

That looks good!

@carlocorradini
Copy link
Contributor

@kf6kjg Nice

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

Next up is looking for / opening a ticket for moving away from the types being in a separate folder to solve the other problem I'm having... :P

@carlocorradini
Copy link
Contributor

What are the other problems?

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

I've created #1442 to track that problem space as it's orthogonal to this ticket. :)

@kf6kjg
Copy link
Author

kf6kjg commented Apr 6, 2023

And the last issue I've got is covered by #1186 - class-validator has roughly the same problems as this repo WRT this ticket and #1442 and thus if it were truly optional I wouldn't have to worry about it.

@MichalLytek
Copy link
Owner

I think this can be closed by #1400 🔒

@carlocorradini Can you share your example repo with ESM usage?

@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Solved ✔️ The issue has been solved labels Aug 9, 2023
@kf6kjg kf6kjg closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Enhancement 🆕 New feature or request Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

3 participants