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

Shared "types" condition for "import" and "require" should not warn if has no default export #63

Closed
bluwy opened this issue Aug 19, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@bluwy
Copy link
Owner

bluwy commented Aug 19, 2023

https://publint.dev/resolve.exports@2.0.2

There should not be a warning for the "types" condition as it's perfectly safe to be shared for both "import" and "require". The "import" would use the CJS-scoped context when interpreting default exports, which can be wrong for ESM, but if the package doesn't default export things, it's fine.

@bluwy bluwy added the bug Something isn't working label Aug 19, 2023
@sapphi-red
Copy link
Sponsor Contributor

sapphi-red commented Aug 19, 2023

IIRC the file needs to use file extensions for imports (vitejs/vite#13947) to share the file for them.
In case of resolve.exports, it's fine as it doesn't contain any import though.

@bluwy
Copy link
Owner Author

bluwy commented Aug 20, 2023

True. Yeah the heuristic to skip this warning then is if it has

  1. No imports (relative imports only)
  2. No default export

I'm thinking we can allow sharing if it's only bare imports as it may still work for some deps. Further analysis for the dep could be done in #18

@bluwy bluwy changed the title Shared "types" condition for "import" and "require" should not warn if has no default export Shared "types" condition for "import" and "require" should not warn if has no default export and relative imports Aug 20, 2023
@bluwy
Copy link
Owner Author

bluwy commented Aug 20, 2023

I think I confused myself. You could import a package that resolves to a CJS dts, and that dts with import statements should be able to import relatively as long as it has the right extension. But the extension is enforced by node16 and bundler itself?

I think I'd want to skip checking whether an import has the extension or not first, since it's harder to track and convey the error message. It would be a new error message e.g. "the dts file contains imports that does not work in node16/bundler".

So regarding sharing the dts itself,

  1. If import resolves to CJS dts, and has default export, warn as it's ambiguous.
  2. If require resolves to ESM dts, and there's corresponding CJS code, warn as it signals you can dynamic import the entrypoint only, even though there's actual CJS code.

Resources:

  1. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md
  2. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md

@bluwy bluwy changed the title Shared "types" condition for "import" and "require" should not warn if has no default export and relative imports Shared "types" condition for "import" and "require" should not warn if has no default export Aug 20, 2023
@sapphi-red
Copy link
Sponsor Contributor

Ah, you're right. Sorry for the confusion.

@bluwy
Copy link
Owner Author

bluwy commented Aug 21, 2023

I'm confusing myself big time 😄 So apparently default export, regardless if one exists or not is also ambiguous, because it's ambiguous at the consumer-side. The attw FalseCJS link already explains that. I'll close this for now.

@bluwy bluwy closed this as not planned Won't fix, can't repro, duplicate, stale Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants