fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 #16

manudeli · 2024-05-31T13:08:48Z

I left comments to share intentions

vercel · 2024-05-31T13:08:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
es-toolkit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 3, 2024 1:10pm

raon0211 · 2024-06-01T08:25:33Z

This is actually intended behavior. (exports will be overriden when it is packed into a tarball) Closing this pull request for the reason.

manudeli · 2024-06-01T08:55:24Z

This is actually intended behavior. (exports will be overriden when it is packed into a tarball) Closing this pull request for the reason.

@raon0211 because of this point, I made this Pull Request. I want to check why we need to override exports field with publishConfig.

For testing or developing this package correctly, if we should use this package in another workspace in this repo or any other places, In my opinion, we should use built JavaScript files, not TypeScript files, to use same files with library users. because TypeScript file can be transpiled differently depending on the user's environment. so I want to ensure that library maintainer and library users use same files, and I believe that is an important point in the process of developing a library.

raon0211 · 2024-06-01T11:41:21Z

In my opinion, when this package is published, library users only see the built JavaScript file, not the TypeScript code. Therefore, all library users interact with the JavaScript code, not the TypeScript.

I believe the issue arises only when we use the es-toolkit library within this workspace. However, the TypeScript files will be transpiled consistently, and they cannot be transpiled differently across user environments, since we control all the compile options within this workspace.

Additionally, using TypeScript directly greatly simplifies the development process. For instance, we don't have to build JavaScript every time we change the source code. Personally, I think this is the direction the JavaScript ecosystem is heading. We have the types as comments proposal in TC39 (as a language spec), and some JavaScript runtimes like Deno natively support TypeScript.

What do you think?

This reverts commit f8534a1.

manudeli · 2024-06-01T15:47:00Z

Personally, I think this is the direction the JavaScript ecosystem is heading. We have the types as comments proposal in TC39 (as a language spec), and some JavaScript runtimes like Deno natively support TypeScript.

Thanks, I understood why you want to override exports field with publishConfig. I reverted it in 3281a7a

but we should fix these errors https://arethetypeswrong.github.io/?p=es-toolkit%401.0.2.

types field is required in my opinion to support TypeScript users.

package.json

.gitignore

.scripts/postbuild.sh

raon0211 · 2024-06-03T13:07:39Z

Thanks for your recommendation for arethetypeswrong.io! I nearly fixed our type imports as the following image.

However it is notoriously difficult to fix the "masquerading as CJS" part (since the dual package itself is difficult and adding TypeScript greatly messes it up); I guess this should be fixed with either we integrate tshy (which is not PnP ready) or we go ESM only (after the release of Node.js 22).

Co-authored-by: Jonghyeon Ko <jonghyeon@toss.im>

raon0211

Thanks for your contribution!

manudeli · 2024-06-03T13:10:46Z

Thanks for your contribution!

Wait a moment!

manudeli

@raon0211 Let's fix attw errors..!

manudeli · 2024-06-03T13:11:37Z

package.json

+        "types": "./dist/index.d.ts",
        "import": "./esm/index.mjs",
        "require": "./dist/index.js"


However it is notoriously difficult to fix the "masquerading as CJS" part; I guess this should be fixed with either we integrate tshy (which is not PnP ready) or we go ESM only (after the release of Node.js 22).

In my opnion, this may fix our errors. I learned this way fixing from maintaining @suspensive/react to support cjs, esm for all environments

Suggested change

"types": "./dist/index.d.ts",

"import": "./esm/index.mjs",

"require": "./dist/index.js"

"import": {

"types": "./dist/index.d.ts",

"default": "./esm/index.mjs"

},

"require": {

"types": "./dist/index.d.ts",

"default": "./dist/index.js"

}

Actually it doesn't, because in order to be a "ESM" declaration,

The type's extension should be .mts (and TypeScript's cli does not support changing extensions of declarations)

And for each type declaration (e.g. in ./esm/index.d.mts), we have to specify extensions in its type.

-export * from './array'; +export * from './array/index.mjs';

However this is notoriously difficult in current TypeScript environments unless we introduce some good libraries like tshy. Meanwhile tshy does not support Yarn PnP.

then How about using tsup? I think tsup support this quite easily!
https://github.com/suspensive/react/blob/main/packages/react/package.json#L28-L36

@raon0211 Fast merging to fix TypeScript error and next Pull Request for tsup is also good for me. If you agree tsup way, I'll make new Pull Request for this!

Actually tsup does not support our usecase neither.

In order to be tree-shaken well, it is necessary for us to use code splitting. However I find that tsup (and its internal implemenation, esbuild) does not support code splitting properly as rollup's preserveModules.

For example, while rollup completely splits our JavaScript files like this:

esbuild's splitting option only makes some chunks and does not split our project files completely.

Maybe we might try just using tsup's declaration generation...

raon0211 · 2024-06-03T13:30:28Z

Will merge this pull request, and will handle integration of tsup as a seperate one. Thanks for your contribution!

po4tion · 2024-07-03T04:23:41Z

Hello @raon0211 !
I have a question. Why do we need this file(.scripts/postbuild.sh)?

fix: remove unnecessary exports overriding

f8534a1

manudeli self-assigned this May 31, 2024

manudeli marked this pull request as ready for review May 31, 2024 13:10

manudeli requested a review from raon0211 May 31, 2024 13:10

raon0211 closed this Jun 1, 2024

manudeli reopened this Jun 1, 2024

Merge branch 'main' into fix/exports/remove-overriding

ab2f0a3

vercel bot deployed to Preview June 1, 2024 08:56 View deployment

Merge branch 'main' into fix/exports/remove-overriding

faf60b4

vercel bot deployed to Preview June 1, 2024 15:28 View deployment

manudeli added 2 commits June 2, 2024 00:36

Revert "fix: remove unnecessary exports overriding"

3281a7a

This reverts commit f8534a1.

chore(package.json): add types field in publishConfig.exports

4948f2e

vercel bot deployed to Preview June 1, 2024 15:44 View deployment

manudeli commented Jun 1, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

manudeli changed the title ~~fix(package.json): remove unnecessary exports overriding~~ fix(package.json): add types field Jun 1, 2024

raon0211 added 2 commits June 3, 2024 21:36

build: Support moduleResolution: node10 and node16 (nodenext)

a14bcb1

build: Support moduleResolution: node10 and node16 (nodenext)

8d96b0e

vercel bot had a problem deploying to Preview June 3, 2024 13:04 Failure

build: Support moduleResolution: node10 and node16 (nodenext)

a00ed8d

manudeli commented Jun 3, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

vercel bot deployed to Preview June 3, 2024 13:06 View deployment

manudeli commented Jun 3, 2024

View reviewed changes

.scripts/postbuild.sh Outdated Show resolved Hide resolved

raon0211 and others added 2 commits June 3, 2024 22:09

fix delay

c2b2a47

Update .gitignore

7974c90

Co-authored-by: Jonghyeon Ko <jonghyeon@toss.im>

Update .scripts/postbuild.sh

4987ef9

Co-authored-by: Jonghyeon Ko <jonghyeon@toss.im>

vercel bot deployed to Preview June 3, 2024 13:10 View deployment

raon0211 approved these changes Jun 3, 2024

View reviewed changes

raon0211 changed the title ~~fix(package.json): add types field~~ fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 Jun 3, 2024

manudeli commented Jun 3, 2024

View reviewed changes

manudeli requested a review from raon0211 June 3, 2024 13:15

raon0211 merged commit 848f2c6 into main Jun 3, 2024
7 checks passed

manudeli deleted the fix/exports/remove-overriding branch June 3, 2024 13:30

This was referenced Jun 3, 2024

Maybe we might try just using tsup's declaration generation... #20

Closed

fix(*): migrate rollup to tsup, support esm correctly, remove *.spec.* for dist #21

Merged

po4tion mentioned this pull request Jul 3, 2024

Why do we need postbuild.sh? #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 #16

fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 #16

manudeli commented May 31, 2024 •

edited

Loading

vercel bot commented May 31, 2024 •

edited

Loading

raon0211 commented Jun 1, 2024

manudeli commented Jun 1, 2024 •

edited

Loading

raon0211 commented Jun 1, 2024

manudeli commented Jun 1, 2024

raon0211 commented Jun 3, 2024 •

edited

Loading

raon0211 left a comment

manudeli commented Jun 3, 2024

manudeli left a comment

manudeli Jun 3, 2024 •

edited

Loading

raon0211 Jun 3, 2024 •

edited

Loading

manudeli Jun 3, 2024

manudeli Jun 3, 2024 •

edited

Loading

raon0211 Jun 3, 2024 •

edited

Loading

raon0211 Jun 3, 2024

raon0211 commented Jun 3, 2024

po4tion commented Jul 3, 2024

fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 #16

fix(package.json): Provide correct types for moduleResolution: node, node10, and node16 #16

Conversation

manudeli commented May 31, 2024 • edited Loading

vercel bot commented May 31, 2024 • edited Loading

raon0211 commented Jun 1, 2024

manudeli commented Jun 1, 2024 • edited Loading

raon0211 commented Jun 1, 2024

manudeli commented Jun 1, 2024

raon0211 commented Jun 3, 2024 • edited Loading

raon0211 left a comment

Choose a reason for hiding this comment

manudeli commented Jun 3, 2024

manudeli left a comment

Choose a reason for hiding this comment

manudeli Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

raon0211 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

manudeli Jun 3, 2024

Choose a reason for hiding this comment

manudeli Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

raon0211 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

raon0211 Jun 3, 2024

Choose a reason for hiding this comment

raon0211 commented Jun 3, 2024

po4tion commented Jul 3, 2024

manudeli commented May 31, 2024 •

edited

Loading

vercel bot commented May 31, 2024 •

edited

Loading

manudeli commented Jun 1, 2024 •

edited

Loading

raon0211 commented Jun 3, 2024 •

edited

Loading

manudeli Jun 3, 2024 •

edited

Loading

raon0211 Jun 3, 2024 •

edited

Loading

manudeli Jun 3, 2024 •

edited

Loading

raon0211 Jun 3, 2024 •

edited

Loading