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 types for NodeNext #2365

Closed

Conversation

benasher44
Copy link
Contributor

What issue does this pull request resolve?

Closes #2132

What changes did you make?

Changed the way types are exported to improve support for users module: node16/nodenext users.

Is there anything that requires more attention while reviewing?

lib/ajv.ts Outdated Show resolved Hide resolved
@benasher44 benasher44 marked this pull request as ready for review January 8, 2024 23:59
@benasher44
Copy link
Contributor Author

@epoberezkin I think this is ready for review now, and I'm now just hitting the node 14 build issue plaguing the main branch. Maybe we can disable now that node 14 is EOL?

@benasher44
Copy link
Contributor Author

Okay yep this is ready. You can see the build passing on the other builds in b7b5fd0, where the only change I did was temporarily remove 14 from the matrix.

import * as runtimeValidationError from "./runtime/validation_error"
import * as compileRefError from "./compile/ref_error"

// eslint-disable-next-line @typescript-eslint/no-namespace, no-redeclare
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems typescript-eslint isn't very aware of the export import feature in namespaces typescript-eslint/typescript-eslint#4129

@benasher44
Copy link
Contributor Author

Pushed more updates + confirmed the build pass without node 14

@benasher44
Copy link
Contributor Author

Thanks @jasoniangreen! Let me know if you have feedback on this PR — would love to move this forward

@jasoniangreen
Copy link
Collaborator

Thanks @jasoniangreen! Let me know if you have feedback on this PR — would love to move this forward

Let me update your branch with master and have a look.

@jasoniangreen
Copy link
Collaborator

jasoniangreen commented Feb 29, 2024

Something that hasn't been considered with this change is the browser bundles we generate. With this PR the rollup command, which is used to generate browser bundles, doesn't work.
Also there are a lot of changes in this PR, and after speaking to epoberezkin there is not a lot appetite for such a large change to resolve this, at least not at this time. He has asked me to explore some alternatives, possible based on this work around.

Either way we have to be wary of the various exports and ways that AJV is used and ensure we don't make life difficult for a lot of people by changing things unnecessarily.

@benasher44
Copy link
Contributor Author

That's totally fair! Would you consider just adding named exports for the classes alongside the default exports? The main pain point is the default exports. If people have the named exports as an alternative, I think that would satisfy most folks, even if it means the default exports are left clunky.

@benasher44
Copy link
Contributor Author

Opened #2389, which is much smaller. NodeNext users can switch to using the named export instead, side-stepping the default import/export issues.

@benasher44
Copy link
Contributor Author

Closing in favor of #2389

@benasher44 benasher44 closed this Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Typescipt and ESM yields "Ajv This expression is not constructable"
3 participants