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

Verify export = vs export default for CJS #10

Closed
remcohaszing opened this issue Mar 12, 2023 · 5 comments · Fixed by #85
Closed

Verify export = vs export default for CJS #10

remcohaszing opened this issue Mar 12, 2023 · 5 comments · Fixed by #85
Labels
bug Something isn't working planned I'm going to do this

Comments

@remcohaszing
Copy link
Contributor

A common mistake people make is to write or emit module.exports = thing in the CJS output, but export default thing in the type definitions. It would be nice if this tool could handle that and provide suggestions how to fix it.

This involves inspecting the actual source code instead of just file resolutions, so I understand if this is a big ask, or even out of scope of this project.

@remcohaszing remcohaszing changed the title Verify export = vs export default = for CJS Verify export = vs export default for CJS Mar 12, 2023
@andrewbranch
Copy link
Collaborator

Is there an example of this in a real package I can look at?

@remcohaszing
Copy link
Contributor Author

remcohaszing commented Apr 13, 2023

postcss-image-set-function uses module.exports =, but its type definitions specify export default.

https://arethetypeswrong.github.io/?p=postcss-image-set-function@5.0.2

@andrewbranch
Copy link
Collaborator

Ah, I thought this would be an easy fix, but it looks like I should probably address this note:

// Also need to check for `default` property on `jsModule["export="]`?

before expanding the scope of this rule, which requires bringing the type checker into play, like #22 also requires. Turning the rule on for other resolution modes (I think it was an accident that I didn’t before) made postcss really blow up, because it can’t identify the pattern:

module.exports = AtRule
AtRule.default = AtRule

that they use throughout, which should not trigger the FalseDefaultExport problem.

I started some work on type checking / whole program support a few days ago, but I’m still just working on this in my spare time.

@remcohaszing
Copy link
Contributor Author

Another one is classnames@2.3.0. (It’s fixed in newer versions.) It’s reported as an issue for ESM, but really it’s an issue for CJS.

When compiling a default import to CJS consuming that package, those types are fine. However, when manually writing CJS and using checkJs, it’s not nice to use such types.

It’s different though, because unlike postcss-image-set-function, classnames@2.3.0 at least could be used without esModuleInterop.

@andrewbranch andrewbranch added bug Something isn't working planned I'm going to do this labels Jun 11, 2023
@not-my-profile
Copy link
Contributor

not-my-profile commented Jul 4, 2023

I think it would make sense to display a ? for the node16 (from CJS) row instead of a ✅ until this issue has been fixed since claiming that the types are ok when they're not is misleading and confusing.

Or perhaps add a disclaimer that all ✅ are to be taken with a grain of salt since this tool doesn't yet support type checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working planned I'm going to do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants