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

Add named exports for main classes to improve types for NodeNext #2389

Merged

Conversation

benasher44
Copy link
Contributor

@benasher44 benasher44 commented Mar 1, 2024

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.

@benasher44
Copy link
Contributor Author

The idea is that once this ships, users having trouble with the default export can switch to the name export instead.

@jasoniangreen
Copy link
Collaborator

Thanks for coming up with a simpler solution, but just so I understand, why do you need to both add export to export class Ajv2019 extends AjvCore as well as adding the .Ajv2019 to exports? And will adding the export keyword conflict with module.exports = exports = Ajv2019?

@benasher44
Copy link
Contributor Author

benasher44 commented Mar 2, 2024

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:

exports.Ajv = Ajv;
module.exports = exports = Ajv;
module.exports.Ajv = Ajv;

First, exports.Ajv = Ajv is set and appears to be emitted by tsc. The problem is that the next line module.exports = exports = Ajv immediately overwrites that. Without any changes, this would cause a named import of Ajv to crash at runtime, since the Ajv class itself does not come with a static prop (or otherwise) to support the named export. This module.exports = exports = Ajv line comes from ajv.ts directly, so tsc is forced to emit it (afaict), despite (and probably unbeknownst to tsc) that is overwrites the named export. To counteract that, I added module.exports.Ajv = Ajv on the following line, which effectively adds an Ajv prop to the class, re-enabling the named export.

@jasoniangreen
Copy link
Collaborator

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.

@jasoniangreen
Copy link
Collaborator

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.

@benasher44
Copy link
Contributor Author

No problem. Thanks!

@jomaa-daniel
Copy link

Waiting on this update as well. 🙏

@jasoniangreen
Copy link
Collaborator

@epoberezkin FYI

@jasoniangreen
Copy link
Collaborator

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.

Copy link
Collaborator

@jasoniangreen jasoniangreen left a 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.

@jasoniangreen jasoniangreen merged commit f4a4c8e into ajv-validator:master Mar 29, 2024
5 checks passed
@flowluap
Copy link

flowluap commented Mar 31, 2024

FYI:

I used those two commits, to patch the package for us, pre release. I am still getting:

TS2709: Cannot use namespace  Ajv  as a type.

This is how I import it...

import Ajv from 'ajv';

Could you include a named export for Ajv, usable as type?

@benasher44
Copy link
Contributor Author

The expectation is you would change the import to:

import { Ajv } from 'ajv';

@flowluap
Copy link

@benasher44 thanks for the fast reply. I installed the latest PR regarding the topic:

 "ajv": "github:ajv-validator/ajv#pull/2365/head",

```

And that one seems to be working perfectly fine with

```

 "lib": ["es2023"],
 "module": "node16",
 "moduleResolution": "nodenext",
```

Looking forward to the new release, thanks for your work!

@benasher44
Copy link
Contributor Author

👍 can you try with this PR and the named import I suggested? I don't expect that #2365 will ship, at this point.

@flowluap
Copy link

With the PR installed, using the named import like that:

import { Ajv } from 'ajv';

/// TS2595:  Ajv  can only be imported by using a default import.

Working fine is the default:

import  Ajv  from 'ajv';

@benasher44
Copy link
Contributor Author

benasher44 commented Apr 1, 2024

@flowluap I'm not sure what you're doing, but if I pull master, run npm i, then npm run build, then npm run pack, I get a usable package. From there, I installed the locally built package. With that installed, I can import { Ajv } from 'ajv' without issue.

@ehaynes99
Copy link

For anyone who needs a workaround until this is released, this works and is typesafe. There are similar problems with ajv-errors and ajv-formats, and those haven't been published in forever.

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)

@emmenko
Copy link

emmenko commented Apr 12, 2024

Thanks for fixing this! ❤️

Is there a plan when this might be released? 🙏

@shumphrey
Copy link

shumphrey commented Apr 30, 2024

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?

@benasher44
Copy link
Contributor Author

What's the issue?

@shumphrey
Copy link

What's the issue?

Argument of type 'import("blank/node_modules/ajv/dist/ajv").Ajv' is not assignable to parameter of type 'import("blank/node_modules/ajv-formats/node_modules/ajv/dist/core").default

This is with various different versions of import addFormats from "ajv-formats";

Seems like the type has changed and is no longer compatible?

@shumphrey
Copy link

shumphrey commented Apr 30, 2024

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

@benasher44
Copy link
Contributor Author

Can you please post the typescript error?

@benasher44
Copy link
Contributor Author

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());

@shumphrey
Copy link

npm ls ajv
│ ├─┬ ajv-formats@3.0.1
│ │ └── ajv@8.12.0
│ └── ajv@8.13.0

I believe ajv-formats was bringing in the old ajv, resetting the package-lock resolved it.

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.

7 participants