-
-
Notifications
You must be signed in to change notification settings - Fork 876
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
Add named exports for main classes to improve types for NodeNext #2389
Add named exports for main classes to improve types for NodeNext #2389
Conversation
395590a
to
ac2347f
Compare
The idea is that once this ships, users having trouble with the default export can switch to the name export instead. |
Thanks for coming up with a simpler solution, but just so I understand, why do you need to both add |
Good questions! For Ajv2019 (and Ajv2020), that was just for consistency. I don't use them, but without much context, it felt weird to not do it for the others. I'm happy undo those! As for the module.exports bit, let's look at the final output (dist/ajv.js), for example. Here's the relevant snippet:
First, |
Thanks for the explanation, I will discuss with EP and get back to you. It might take me a couple of days as I have another priority at the moment, but I will definitely get back to you soon. |
I haven't forgotten you. Just trying to get a release together with updated deps (as it has been a while), and then I will follow up on this with EP. |
No problem. Thanks! |
Waiting on this update as well. 🙏 |
@epoberezkin FYI |
I spoke to EP, I will be preparing a release with this and general dependency updates over the coming days. He has asked me to do some very specific due diligence and tests to reduce the chance of any issues. We have to be super cautious given the huge user base that rely on AJV and the wide variety of ways it is used, imported and built into projects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will include this in the next release. There are some manual steps and some extra testing that EP has asked me to conduct, so no timeline yet.
FYI: I used those two commits, to patch the package for us, pre release. I am still getting:
This is how I import it...
Could you include a named export for Ajv, usable as type? |
The expectation is you would change the import to:
|
@benasher44 thanks for the fast reply. I installed the latest PR regarding the topic:
|
👍 can you try with this PR and the named import I suggested? I don't expect that #2365 will ship, at this point. |
With the PR installed, using the named import like that:
Working fine is the default:
|
@flowluap I'm not sure what you're doing, but if I pull master, run |
For anyone who needs a workaround until this is released, this works and is typesafe. There are similar problems with import AjvLib from 'ajv'
import AjvErrorsLib from 'ajv-errors'
import AjvFormats from 'ajv-formats'
const { default: Ajv } = AjvLib
type Ajv = InstanceType<typeof Ajv>
const { default: ajvErrors } = AjvErrorsLib
const { default: addFormats } = AjvFormats
const ajv: Ajv = new Ajv({ allErrors: true, strict: false })
ajvErrors(ajv, { singleError: true })
addFormats(ajv) |
Thanks for fixing this! ❤️ Is there a plan when this might be released? 🙏 |
Now that this has been released, I'm having issues getting ajv-formats working with this change, does a similar change need to be made to ajv-formats? |
What's the issue? |
This is with various different versions of Seems like the type has changed and is no longer compatible? |
The old typescript code was: import Ajv from "ajv";
import addFormats from "ajv-formats";
const ajv = new Ajv.default({ allowUnionTypes: true });
addFormats.default(ajv); With the new release of ajv: import { Ajv } from "ajv";
import addFormats from "ajv-formats";
const ajv = new Ajv({ allowUnionTypes: true });
addFormats.default(ajv); // this throws a typescript error I've tried various combos of how we import ajv-formats, I think the typing of the two don't work together any more |
Can you please post the typescript error? |
This works fine for me with ajv-formats 3.01 and ajv 8.13.0. Can you please post a reproduction? import { Ajv } from "ajv";
import addFormats from "ajv-formats";
addFormats.default(new Ajv()); |
I believe ajv-formats was bringing in the old ajv, resetting the package-lock resolved it. |
What issue does this pull request resolve?
Users of TypeScript + ESM + NodeNext have difficulty using the default exported classes, which is the only way to consume them (#2132).
What changes did you make?
This additionally adds a named export, which side-steps this issues around default imports in that environment. This is a very lightweight alternative to #2365 to improve things for these users, even if it won't make this package pass attw.
Is there anything that requires more attention while reviewing?
I don't think so.