Skip to content

Add "types" to "exports"#1704

Merged
shadowspawn merged 2 commits intotj:developfrom
ChocolateLoverRaj:patch-1
Mar 18, 2022
Merged

Add "types" to "exports"#1704
shadowspawn merged 2 commits intotj:developfrom
ChocolateLoverRaj:patch-1

Conversation

@ChocolateLoverRaj
Copy link
Copy Markdown
Contributor

@ChocolateLoverRaj ChocolateLoverRaj commented Mar 16, 2022

Pull Request

Problem

Fixes #1703

Solution

Add "types" to "exports"

ChangeLog

shadowspawn
shadowspawn requested changes Mar 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@shadowspawn shadowspawn left a comment •
edited
Loading

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
Copy Markdown
Collaborator

shadowspawn commented Mar 17, 2022

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 shadowspawn mentioned this pull request Mar 17, 2022
Closed
@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Mar 17, 2022

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

@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Mar 17, 2022

And got a "100% yes".

microsoft/TypeScript#46452 (comment)

@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Mar 17, 2022

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

microsoft/TypeScript#46452 (comment)

@ChocolateLoverRaj
Copy link
Copy Markdown
Contributor Author

ChocolateLoverRaj commented Mar 17, 2022

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

@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Mar 18, 2022 •
edited
Loading

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
Copy Markdown
Collaborator

shadowspawn commented Mar 18, 2022

@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.

shadowspawn
shadowspawn approved these changes Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
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

abetomo
abetomo approved these changes Mar 18, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

abetomo commented Mar 18, 2022

I think it is fine to release it next time.

@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Mar 18, 2022

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
@shadowspawn shadowspawn mentioned this pull request Aug 16, 2022
Closed
@bestickley bestickley mentioned this pull request Oct 11, 2022
Closed
2 tasks
@shadowspawn shadowspawn mentioned this pull request May 11, 2023
Closed
@noelforte noelforte mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abetomo abetomo abetomo approved these changes

@shadowspawn shadowspawn shadowspawn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ChocolateLoverRaj @shadowspawn @abetomo