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

fix(javascript): fix types not being recognized for NodeNext module resolution #4540

Merged
merged 3 commits into from
Jul 28, 2024

Conversation

KurtGokhan
Copy link
Contributor

@KurtGokhan KurtGokhan commented Feb 23, 2024

Fixes #4218

The problem was, since the package.json has type: module, Typescript is treating regular d.ts files as module. But the d.ts files are generated manually and they don't comply with how a module file should look like (e.g. missing file extensions on imports). So they are essentially more like CommonJS files.

Luckily, there is a way to override the module detection. If the file has cts or cjs extension, Typescript will treat that file as CommonJS. In this case, adding cts extension only to the index file was enough.

Ideally, the TS files should be built with tsc in the future to ensure the files comply with the corresponding module format.

Also added a files field to the package.json so that only relevant files are included in the NPM bundle. Previously it also included files like spec, src, and dev config files, which made it confusing to debug.

Tested my changes locally with various moduleResolution options like Node, Node10, Node16, NodeNext, Bundler and they work flawlessly. Only the Classic option doesn't work but it also doesn't work on upstream and it doesn't work for most other packages as well.

Alternative solutions

All of the other solutions require changes in more files but I will leave them here as an option.

The first obvious solution is to add extensions to all the TS files.

I checked various other packages like lodash-es and checked why they don't have this problem even though they don't have extension in TS files. Turns out, they don't have type: module in their package.json, so files are treated as CommonJS by default, even though the packages are ES packages.

So one of the solutions is to remove type: module and convert all js files to mjs.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Feb 24, 2024

Interesting, I'd never heard of cts files before, thanks for this.
See my last comment in #4218, hopefully we can get close to a packaging implementation that we can trust.

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Feb 24, 2024

Hopefully we can get close to a packaging implementation that we can trust.

Is the test suite for that absolutely necessary? Imho, we find a working configuration, test it manually in different scenarios, iterate if necessary. This isn't something that will change much. Most npm projects work with this approach.

Also, these changes should not affect runtime at all. This is only a Typescript change, and if needed we can test it with a simple setup of running tsc against different config options. Webpack shouldn't be needed.

Btw, sharing the results from AreTheTypesWrong:

Before:
image

After:
image

Here are the problems:

  • Internal Resolution Error can cause issues if skipLibCheck is false. But this seems to also exist on current setup. But I will check if it can be remedied.
  • Masquerading as CJS isn't a big issue since this package doesn't have any default exports.

@ericvergnaud
Copy link
Contributor

The lack of automated testing is the reason we are where we are. Every now and then, someone comes with a solution to a problem specific to their usage, whilst in parallel the ecosystem keeps evolving. We implement that change and that creates a problem for another use case.

@mike-lischke
Copy link
Member

mike-lischke commented Apr 18, 2024

@KurtGokhan interesting tool you used for this information here. I didn't know that before.

As a test I let it check the new ANTLR4 TypeScript target antlr4ng and it came out as:

Bildschirmfoto 2024-04-18 um 21 38 03

Maybe you want to try that out? I'm open to accept a PR to fix the last remaining issue.

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Apr 18, 2024

@mike-lischke "Masquerading as ESM" shouldn't cause any problems as long as you are not using default exports. It may not need a fix. I will try out your implementation too, thanks.

"node": {
"types": "./src/antlr4/index.d.ts",
"types": "./src/antlr4/index.d.cts",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be removed ?

"import": "./dist/antlr4.node.mjs",
"require": "./dist/antlr4.node.cjs",
"default": "./dist/antlr4.node.mjs"
},
"browser": {
"types": "./src/antlr4/index.d.ts",
"types": "./src/antlr4/index.d.cts",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, removed it and tested with different module resolutions like NodeNext, Bundler and Node. Seems working.

@ericvergnaud
Copy link
Contributor

@parrt let's see if we can get that one in

@ericvergnaud
Copy link
Contributor

@Partt blessed

@parrt parrt added this to the 4.13.2 milestone Jul 28, 2024
@parrt
Copy link
Member

parrt commented Jul 28, 2024

Looks like there are conflicts.

@ericvergnaud
Copy link
Contributor

@parrt I see conflicts if you rebase, not if you merge ?

@parrt parrt merged commit 2559349 into antlr:dev Jul 28, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't import from TypeScript runtime with "moduleResolution": "nodenext"
4 participants