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 "types" to "exports" #1704

Merged
merged 2 commits into from
Mar 18, 2022
Merged

Add "types" to "exports" #1704

merged 2 commits into from
Mar 18, 2022

Conversation

ChocolateLoverRaj
Copy link
Contributor

Pull Request

Problem

Fixes #1703

Solution

Add "types" to "exports"

ChangeLog

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Thanks. The example on this page: https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing

has a comment saying "types" must come first:

            // Entry-point for TypeScript resolution - must occur first!
            "types": "./types/index.d.ts",

(It did work for me last when I was experimenting, but I think we better follow the example.)

@shadowspawn
Copy link
Collaborator

This PR is to work with a future version of TypeScript. I have been doing a lot of reading to try and understand whether it is safe for us to ship now. And still not sure! Maybe I should ask...

  1. The "types" value in "exports" is described in the Node.js 16 documentation so that part at least appears in something released.
    https://nodejs.org/dist/latest-v16.x/docs/api/packages.html#community-conditions-definitions

  2. I found these comments in long threads, two talking about updates by library authors:

Some interesting information about emit vs resolving with answer from an expert:

  1. rxjs have merged a PR doing this just recently, and also wondering whether to go before TypeScript supports it!

@shadowspawn
Copy link
Collaborator

Asked if stable enough for release: microsoft/TypeScript#46452 (comment)

@shadowspawn
Copy link
Collaborator

And got a "100% yes".

microsoft/TypeScript#46452 (comment)

@shadowspawn
Copy link
Collaborator

Oh, but then there was a followup. I need to reread some of the earlier comments again!

microsoft/TypeScript#46452 (comment)

@ChocolateLoverRaj
Copy link
Contributor Author

@shadowspawn How could adding a "types" entry in "exports" break things?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Mar 18, 2022

How could adding a "types" entry in "exports" break things?

In general, by making the behaviour worse for a use case where something reads "exports" and recognises the "types" entry. Not just TypeScript, say ts-node or WebStorm. (Hypothetical examples, I have not idea what they do with "exports"!)

@shadowspawn
Copy link
Collaborator

@abetomo I am going to test now one more time with typescript@next generating CommonJS and ESM, then call it good. Could make it into v9.1, but no worries if it misses.

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

👍

@abetomo
Copy link
Collaborator

abetomo commented Mar 18, 2022

I think it is fine to release it next time.

@shadowspawn
Copy link
Collaborator

Ok. I won't merge until after release we have lined up.

@shadowspawn shadowspawn merged commit 8cbd082 into tj:develop Mar 18, 2022
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Mar 18, 2022
@ChocolateLoverRaj ChocolateLoverRaj deleted the patch-1 branch March 18, 2022 17:40
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Apr 15, 2022
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.

3 participants