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 false positive of FalseExportDefault #67

Merged
merged 2 commits into from
Jul 15, 2023

Conversation

andrewbranch
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jul 15, 2023

🦋 Changeset detected

Latest commit: c8993b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@arethetypeswrong/core Patch
@arethetypeswrong/cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@andrewbranch andrewbranch merged commit f85ba69 into main Jul 15, 2023
@andrewbranch andrewbranch deleted the bug/false-export-default branch July 15, 2023 22:25
@me4502
Copy link

me4502 commented Jul 27, 2023

Was this actually a false positive? TS imports with node16 moduleResolution fail with this library, and there's an issue open on that library around it - ajv-validator/ajv#2132

The suggested fix provided appears to be a workaround for the exact issue that arethetypeswrong was previously flagging

@andrewbranch
Copy link
Collaborator Author

Yes, it is a false positive, because Ajv is available both on module.exports and exports.default:

module.exports = exports = Ajv;
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = Ajv;

This rule is supposed to flag when types declare a default export that isn’t present in the JS. But the “default export” is present here. That means ESM users can reliably do

impor Ajv from "ajv";
const ajv = new Ajv.default();

and the runtime and types agree.

The problem that AJV has is a lesser one that the tool doesn’t yet catch, but would be a good addition. The types should ideally reflect that Ajv is available on export = Ajv, and there should be a default property declared on the Ajv symbol. If the types were written that way, ESM users would be able to do either of these:

import Ajv from "ajv";
new Ajv();
new Ajv.default();

which is the reality at runtime. Detecting this requires running the type checker, though. #10 is related.

@andrewbranch
Copy link
Collaborator Author

@me4502 in case you’re still interested in this, #85 adds a type checker run to bring back a more specific and accurate error for this case with ajv:

image

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 this pull request may close these issues.

2 participants