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 package exports for TypeScript #48

Conversation

jaydenseric
Copy link
Contributor

This PR fixes the package exports field for compatibility with prerelease typescript v4.6. Weirdly, tsc in the CLI didn't have problems resolving the type definition for imports but the VS Code TypeScript extension did.

Project code before, with typescript v4.6.0-dev.20211227:

Screen Shot 2021-12-28 at 11 05 07 am

After the changes in this PR:

Screen Shot 2021-12-28 at 11 06 28 am

The specific change needed for the fix in the package exports field was the addition of exports["."].node.types. The others are added because it might solve other problems. I don't fully understand what the point of the typesVersions field is, so you might want to consider it.

See this TypeScript guide.

For the record, this PR works with the patterns already established but I would setup the files and package exports differently.

@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #48 (36e5c8d) into master (2376841) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          964       964           
  Branches       137       137           
=========================================
  Hits           964       964           
Flag Coverage Δ
unittests 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2376841...36e5c8d. Read the comment docs.

@octet-stream
Copy link
Owner

octet-stream commented Dec 28, 2021

I don't fully understand what the point of the typesVersions field is, so you might want to consider it.

It was introduced as the way to tell TS where to pick up the types for file-from-path, I didn't find another way before and it looks like your PR is introducing the one. So, typesVersion will be removed in a future, but as for now I would rather keep it.
It's weird that TS now can't pick up types from types field. I don't really understand what is going on there and why they can't just pick up what I declared in this field.

For the record, this PR works with the patterns already established but I would setup the files and package exports differently.

What do you mean?

@jaydenseric
Copy link
Contributor Author

What do you mean?

Sorry to tease, but it would be perhaps a 6+ paragraph comment to adequately explain all the nuances and I'm a bit burned out from high-effort comments at the moment.

@octet-stream
Copy link
Owner

It's fine. Take your time, if you would comment on this in a future.
By the way, I'm planning to drop CJS support for this project by the Node.js v12 EOL, so I might need to do something about exports field anyway. Will see.

@octet-stream
Copy link
Owner

Everything looks good so far, but I can't release this immediately because I am busy on my work. Sorry. I'll merge it and include with the next release as soon as I can.

@octet-stream octet-stream merged commit 1b676c9 into octet-stream:master Dec 28, 2021
@jaydenseric jaydenseric deleted the jaydenseric/fix-typescript-package-exports branch December 28, 2021 12:05
@octet-stream
Copy link
Owner

Sorry for the delay :)
This change is now available in https://github.com/octet-stream/form-data/releases/tag/v4.3.2

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