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

Doesn’t check that imports inside the package work #22

Closed
jedwards1211 opened this issue Apr 3, 2023 · 12 comments · Fixed by #31
Closed

Doesn’t check that imports inside the package work #22

jedwards1211 opened this issue Apr 3, 2023 · 12 comments · Fixed by #31

Comments

@jedwards1211
Copy link

jedwards1211 commented Apr 3, 2023

When I check [email protected] it appears to show that everything is okay, and I can use that package with node module resolution. But when I try to use that package with nodenext module resolution, I get errors like Module '"antlr4"' has no exported member 'CommonTokenStream'.

CommonTokenStream shows up in the node16-esm resolutions on arethetypeswrong so it seems like it may not be matching the behavior of tsc 5.0.3. That or maybe there's a bug in tsc? The docs don't make it very clear what I should expect tsc to do in this case.

I haven't figured out what I would need to change in a PR to antlr4 but I've been asking around. In addition to needing better package authoring guidance from the docs, it would be good if arethetypeswrong catches problems like this.

@andrewbranch
Copy link
Collaborator

Not every problem is a module resolution / kind detection problem, and this doesn’t really try to go beyond module resolution at this point. What code is giving you an error?

@jedwards1211
Copy link
Author

@andrewbranch just this import in test.ts:

import { CharStream, CommonTokenStream }  from 'antlr4';

I have "type": "module" in my package.json and my tsconfig.json has:

{
  "include": ["test.ts"],
  "compilerOptions": {
    "target": "ESNext",
    "module": "nodenext",
    "moduleResolution": "nodenext",
    "forceConsistentCasingInFileNames": true,
    "strict": false,
    "skipLibCheck": true
  }
}

@jedwards1211
Copy link
Author

jedwards1211 commented Apr 3, 2023

Okay I had a wise idea, I figured out how to enable module resolution tracing and I think I figured out the problem:

======== Resolving module './CharStreams' from '/Users/andy/antlr/node_modules/antlr4/src/antlr4/index.d.ts'. ========
Explicitly specified module resolution kind: 'NodeNext'.
Resolving in ESM mode with conditions 'import', 'types', 'node'.
Loading module as file / folder, candidate module location '/Users/andy/antlr/node_modules/antlr4/src/antlr4/CharStreams', target file types: TypeScript, JavaScript, Declaration.
Directory '/Users/andy/antlr/node_modules/antlr4/src/antlr4/CharStreams' does not exist, skipping all lookups in it.
======== Module name './CharStreams' was not resolved. ========

node_modules/antlr4/src/antlr4/CharStreams.d.ts does exist but the export from statement omits the file extension.

The problem is the .d.ts files in the package don't have file extensions on imports right? I wonder if this is something arethetypeswrong could catch?

@andrewbranch
Copy link
Collaborator

Ouch, yes indeed. Yeah, this kind of check is definitely on my radar. So far, none of the checks use the type checker, so things are pretty fast. This kind of analysis is basically just running tsc on the output, which will take a bit longer, but I think is worth it. Nice find!

@jedwards1211
Copy link
Author

jedwards1211 commented Apr 3, 2023

Ugh I would have caught the issue sooner if tsc --init didn't set "skipLibCheck": true by default... when I turned that off I got this error

node_modules/antlr4/src/antlr4/index.d.ts:19: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.

19 export * from './error';
                 ~~~~~~~~~

@anothermh
Copy link

I would likewise to see nodenext support. For example, I'm troubleshooting GoogleCloudPlatform/functions-framework-nodejs#521 and testing this against @google-cloud/[email protected] would be useful, though I don't know for sure if this tool would be able to help me troubleshoot the problem even if it supported nodenext.

@andrewbranch
Copy link
Collaborator

node16 === nodenext right now.

@andrewbranch andrewbranch changed the title Doesn't identify problems that will occur with nodenext module resolution Doesn’t check that imports inside the package work Apr 25, 2023
@anothermh
Copy link

Thank you, I didn't realize that. It's all green checkmarks on that package so I guess it's back to the drawing board for me.

It's a great tool, thank you for building it.

@andrewbranch
Copy link
Collaborator

I think the issue you’re having is kind of visible in the tool by what’s not there. "@google-cloud/functions-framework" and "@google-cloud/functions-framework/testing" show up as entrypoints, which indicates that there was a package.json "exports" and it didn’t include anything besides those two. The presence of "exports" explicitly prohibits importing subpaths outside of the ones listed in it. So a deep import like @google-cloud/functions-framework/build/src/functions cannot work. It’s not exposed by the package.

You also say in the issue that

This error can be resolved by changing moduleResolution to node instead of nodenext

But be aware that this is Node’s rule, not TypeScript’s, so if you were dealing with runtime imports rather than type-only ones, you would be getting rid of a compile-time error, but would crash at runtime.

@anothermh
Copy link

I really appreciate your expertise here. Thank you for shedding some light on the problem even though it's off-topic from what the original poster asked about.

If I can ask you for your expert opinion, what do you think is the right way to resolve the issue I described so that I would be able to do a deep import? There are many types defined within the recesses of the package that I'd like to be able to access. With a little guidance I would be able to open a PR against the repo to try to encourage some progress from the Google team.

@andrewbranch
Copy link
Collaborator

I think it’s usually good for packages to block arbitrary deep imports, because it lets them define boundaries on their public API, and moving a file that’s supposed to be considered internal doesn’t need to be a breaking change. If those files contain types that are useful, the libraries just need to ensure those types get exported from one of the public API entrypoints.

@anothermh
Copy link

Great answer, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants