Add types reference to package.json exports

[rdunk]

Summary

With TS 5.0 introducing "bundler" module resolution (may also apply to "Node16" and "NodeNext", although I haven't tested), importing types from @vuelidate/core will result in TS complaining that it can't resolve type declarations. TS seems to find the type declaration file but won't use it because it's not explicitly listed under "exports" in package.json.

See this issue for more details: microsoft/TypeScript#52363

This PR just adds a "types" export line to the package.json, which seems to keep TS happy and resolve the error.

Metadata

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• I have read the Contribution Guides
• It's submitted to the next branch
• When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
• All tests are passing: https://github.com/vuelidate/vuelidate/blob/master/.github/CONTRIBUTING.md#development-setup
• New/updated tests are included

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
N/A

[andrewbranch]

This is an improvement, but still incorrect. Two files (.mjs and .cjs) cannot share a single set of type definitions. See “Types are CJS, but implementation is ESM” in https://github.com/arethetypeswrong/arethetypeswrong.github.io.

[dobromir-hristov]

So we should just copy the types into an .mts file?

[rdunk]

@andrewbranch thanks very much for taking a look and for pointing out my mistake! I believe the exported types have been written rather than generated from TS files, so as @dobromir-hristov pointed out would this just be a case of copying and altering the file extension? In what way would the types differ in this instance?

[andrewbranch]

A copy/paste of the content may or may not result in a valid type declaration file. As you probably know, in Node, imports in ES modules use a different module resolution algorithm from requires in CJS modules. Type declaration files are assumed to be auto-generated, and when they’re generated by tsc, they’re a (near) perfect representation of the corresponding JS. So when a declaration file for a CommonJS module says

import { foo } from "./foo"

we know the JS looks something like

const foo_1 = require("./foo")

and we can verify that the module specifier "./foo" would resolve successfully in a require to another CommonJS module that has a foo member on its exports. If you copy that to a declaration file for an ES module, however, then we assume that the corresponding JS looks like

import { foo } from "./foo"

And here, we need to verify that the module specifier "./foo" would resolve successfully in a Node ESM import to either a CommonJS module with a foo member on its exports or another ES module with a foo named export. And in this case, this import will not resolve, because Node ESM does not allow for dropping file extensions the way CommonJS require does. This module specifier would have to be written as "./foo.js" or "./foo/index.js" or whatever the actual relative path to the module is. Again, this is Node’s rule, and TypeScript is only enforcing it because the declaration file is making a claim about what’s present in the JS file. A declaration file that looks like this indicates to TypeScript that the runtime JS is going to crash in Node, so it issues a compiler-time error to protect from that.

So you can see that identical content needs to produce two different checking behaviors, and that the content needs to match the runtime JS very closely to avoid false positives. Sometimes, a copy/paste actually works fine (but due to the file extension restriction, it’s usually copying ESM declarations to CJS that’s more likely to work). You can perhaps start with a copy/paste and then run tsc --module nodenext on them to validate. I also suggest using https://arethetypeswrong.github.io to check your solution before publishing.

[rdunk]

Checking @vuelidate/core currently with https://arethetypeswrong.github.io/ produces the following:

The change I've made in this PR, tested using npm pack produces:

If I build the package, duplicate index.d.ts twice, to dist/index.d.cts and dist/index.d.mts, and update package.json (remove the "exports"."types" change I made and update "types" to "dist/index.d.cts"), then I see the following:

Whether or not those type definitions are actually correct I do not know. As they are not auto-generated I assume there is no meaningful difference, but that my last solution is just declaring them in the "correct" way? If that's the best solution then I guess it would just be a case of updating the build process to duplicate and rename the type definition file into the dist directory, which I'd be happy to do in this PR. Any ideas @dobromir-hristov?

[andrewbranch]

I checked the existing type declarations manually and can confirm the existing .d.ts looks accurate for both the .mjs and .cjs file. Should be fine to copy/paste like that. Thanks @rdunk!

[dobromir-hristov]

Then we need to just add a command to the build I guess? To copy the types with a new name.

[rdunk]

@andrewbranch Really appreciate all your input on this, thanks very much.

@dobromir-hristov Yep I believe so, I'm happy to make the change here if you'd like, do you have any preference? Something like rollup-plugin-copy would be simplest but I guess requires an extra dependency, so happy to use a script with fs.copyFile() or something if you'd prefer.

[rob-pace]

This would definitely be handy for us to merge in, if possible!

[dobromir-hristov]

Sorry for the incredibly late reply. A simple copy command could work but ppl on different OS would have issues. Maybe it's better to add a plugin as it's already meant to work everywhere.

[rdunk]

@dobromir-hristov No problem at all. I will add that in and update the PR.

[rdunk]

I've added in a config flag to the generateConfigFactory function which specifies if types should be copied (using rollup-plugin-copy), and set that to true for both the vuelidate and validators packages.

[dobromir-hristov]

Looks great. Should we change the exports field?

https://github.com/vuelidate/vuelidate/pull/1184/files#diff-b50ff1151947a06df858e0c4ad805bc7bdf958351015760599f9384d1bb7a984R19-R25

[dobromir-hristov]

Could changing the extension break older versions of typescript?

[rdunk]

Ah, yeah maybe it could... So I looked at the documentation here and just pushed two changes to the package.json files based on the last example given on that page:

1. Reverted "files" to include the uncopied index.d.ts file again, and reverted the "types" field to reference that file. So basically no change to those fields from before this PR.
2. Updated the "exports" field to include the new type declaration files alongside the default import conditions.

I've found this a tad confusing (three identical type declaration files now?) so apologies if I've not got it right again, but arethetypeswrong seems to be happy. Ultimately all this PR now does is copy the type declaration twice, and reference those files in the package.json "exports". So maybe the solution was quite simple after all.

Maybe @andrewbranch can give us a final verification on if this is an OK resolution?

[andrewbranch]

Yeah, TS versions/configurations that understand exports (and the newer file extensions) won’t look at this top-level types field since they’ll look in exports instead, so it’s best practice to leave the top-level types as a .d.ts (as long as you have exports) for the benefit of old TS versions.

[rdunk]

Great, thanks for the clarification, that's really helpful to know.

[dobromir-hristov]

LGTM

[Ghosterbeef]

LGTM

When should we expect a release with this fix?

[yakobe]

@dobromir-hristov would it be possible to get a release with this change? Many thanks 😸

[dobromir-hristov]

Sorry bout that, busy week. Try now.

[rob-pace]

❤️