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

chore!: switch to tsup #2391

Merged
merged 2 commits into from
Feb 16, 2024
Merged

chore!: switch to tsup #2391

merged 2 commits into from
Feb 16, 2024

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Sep 11, 2023

This PR switches the build process from using esbuild nativly to tsup (which uses esbuild under the hood).
It simplifies the config and prepares for easier migration later to esm (see #2261)

And even better! It magically reduces the dist folder content:

esbuild -> tsup
14.4 MiB (15,053,311) -> 10.6 MiB (11,066,749)
5,715 files -> 3,072 files
1,210 sub-folders -> 605 sub-folders

@Shinigami92 Shinigami92 added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent c: dependencies Pull requests that adds/updates a dependency c: infra Changes to our infrastructure or project setup labels Sep 11, 2023
@Shinigami92 Shinigami92 self-assigned this Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (44d698e) 99.56% compared to head (a9d26a8) 99.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2391      +/-   ##
==========================================
- Coverage   99.56%   99.55%   -0.01%     
==========================================
  Files        2817     2817              
  Lines      251199   251199              
  Branches     1129     1125       -4     
==========================================
- Hits       250101   250088      -13     
- Misses       1069     1111      +42     
+ Partials       29        0      -29     

see 30 files with indirect coverage changes

@Shinigami92 Shinigami92 marked this pull request as ready for review September 11, 2023 18:17
@Shinigami92 Shinigami92 requested a review from a team September 11, 2023 18:17
@Shinigami92 Shinigami92 requested a review from a team as a code owner September 11, 2023 18:17
@ST-DDT
Copy link
Member

ST-DDT commented Sep 11, 2023

And even better! It magically reduces the dist folder content:
esbuild -> tsup
14.4 MiB (15,053,311) -> 10.6 MiB (11,066,749)
5,715 files -> 3,072 files
1,210 sub-folders -> 605 sub-folders

Mind giving some details what/why files/data are now missing?

@Shinigami92
Copy link
Member Author

Mind giving some details what/why files/data are now missing?

I was hoping you'd want to check out the PR and discover it for yourself, but okay, I can spoil it for you:

As you can already see in the package.json

faker/package.json

Lines 31 to 53 in 5445afe

"main": "dist/index.js",
"module": "dist/index.mjs",
"types": "index.d.ts",
"typesVersions": {
">=4.0": {
"*": [
"dist/types/*"
]
}
},
"exports": {
".": {
"types": "./dist/types/index.d.ts",
"require": "./dist/index.js",
"import": "./dist/index.mjs"
},
"./locale/*": {
"types": "./dist/types/locale/*.d.ts",
"require": "./dist/locale/*.js",
"import": "./dist/locale/*.mjs"
},
"./package.json": "./package.json"
},

the main, module and exports fields have changed.
So the files are no longer in separate folders but side-by-side.
This already splits the sub-folders count into half.

The other changes comes from (🥁): tsup supports splitting for cjs 🎉

@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2023

Well, not only does tsup support splitting, but also chunking (Hence the reduced number of files for cjs).

So for cjs users it is no longer possible to import directly from .../dist/cjs/locales/af_ZA/cell_phone/formats.js.
(Not that I would recommend doing so anyway)
Do we expect any of our users to use that kind of import paths?

@Shinigami92
Copy link
Member Author

Well, not only does tsup support splitting, but also chunking (Hence the reduced number of files for cjs).

// chunking == splitting

So for cjs users it is no longer possible to import directly from .../dist/cjs/locales/af_ZA/cell_phone/formats.js. (Not that I would recommend doing so anyway) Do we expect any of our users to use that kind of import paths?

No! Anything not defined in exports is NOT supported.

@matthewmayer
Copy link
Contributor

Well, not only does tsup support splitting, but also chunking (Hence the reduced number of files for cjs).

// chunking == splitting

So for cjs users it is no longer possible to import directly from .../dist/cjs/locales/af_ZA/cell_phone/formats.js. (Not that I would recommend doing so anyway) Do we expect any of our users to use that kind of import paths?

No! Anything not defined in exports is NOT supported.

while its not supported, its still a breaking change, so i think it should land in 9.0

@Shinigami92 Shinigami92 modified the milestones: vAnytime, v9 - Next major Sep 13, 2023
package.json Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Sep 13, 2023
@matthewmayer matthewmayer added the breaking change Cannot be merged when next version is not a major release label Sep 14, 2023
import-brain
import-brain previously approved these changes Sep 15, 2023
@ST-DDT ST-DDT added the do NOT merge yet Do not merge this PR into the target branch yet label Sep 15, 2023
tsup.config.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Feb 9, 2024
@Shinigami92 Shinigami92 marked this pull request as draft February 9, 2024 15:57
@Shinigami92
Copy link
Member Author

I checked if it is viable to ship sourcemaps, but hell no 😆

// With sourcemaps:
22.0 MiB (23,026,364)
580 files, 610 sub-folders

// Without sourcemaps:
10.0 MiB (10,437,487)
290 files, 610 sub-folders

12 MiB is NOT worth it 😬

@Shinigami92 Shinigami92 changed the title chore: switch to tsup chore!: switch to tsup Feb 9, 2024
@Shinigami92 Shinigami92 marked this pull request as ready for review February 9, 2024 16:14
ST-DDT
ST-DDT previously approved these changes Feb 9, 2024
ST-DDT
ST-DDT previously approved these changes Feb 9, 2024
@ST-DDT ST-DDT requested a review from a team February 9, 2024 17:40
@Shinigami92
Copy link
Member Author

@ST-DDT already approved a week ago and only a rebase were done since then
We count this a one-week approval by only one maintainer, but dont violate our merge rules

@Shinigami92 Shinigami92 enabled auto-merge (squash) February 16, 2024 15:20
@Shinigami92 Shinigami92 merged commit db88a15 into next Feb 16, 2024
30 checks passed
@ST-DDT ST-DDT deleted the switch-to-tsup branch February 25, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: chore PR that doesn't affect the runtime behavior c: dependencies Pull requests that adds/updates a dependency c: infra Changes to our infrastructure or project setup p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants