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

infra: replace tsup with packem #3094

Open
wants to merge 11 commits into
base: next
Choose a base branch
from
Open

Conversation

prisis
Copy link
Member

@prisis prisis commented Sep 7, 2024

This is a different approach to fix #3087

  • Switched tsup with packem
  • Reduced package size from npm notice package size: 2.1 MB npm notice unpacked size: 8.6 MB to npm notice package size: 2.0 MB npm notice unpacked size: 6.8 MB (hard to display, the newest version has more type files..) -> run npm pack --dry-run to validate it
  • Fixed types for
    • "@faker-js/faker" node10 node16 (from CJS) node16 (from ESM) bundler
    • "@faker-js/faker/locale/*" node10 node16 (from CJS) node16 (from ESM) bundler
    • "@faker-js/faker/package.json" node10 node16 (from CJS) node16 (from ESM) bundler
  • Changed exports to support the new types files
  • The typesVersions are generated based on https://github.com/andrewbranch/example-subpath-exports-ts-compat/tree/main/examples/node_modules/types-versions-wildcards, this is handle by packem

@prisis prisis requested a review from a team as a code owner September 7, 2024 10:47
Copy link

netlify bot commented Sep 7, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 58a8fa7
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e0a9e124009d0008ab9f38
😎 Deploy Preview https://deploy-preview-3094.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Sep 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (f128d77) to head (58a8fa7).
Report is 18 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3094      +/-   ##
==========================================
+ Coverage   99.95%   99.96%   +0.01%     
==========================================
  Files        2776     2776              
  Lines      226260   226260              
  Branches      587      593       +6     
==========================================
+ Hits       226164   226187      +23     
+ Misses         96       73      -23     

see 1 file with indirect coverage changes

@ST-DDT
Copy link
Member

ST-DDT commented Sep 7, 2024

Node10 works fine for me, what needs to be fixed?

@prisis prisis changed the title fix: fixed support for node10 and reduced bundle size draft: fix: fixed support for node10 and reduced bundle size Sep 7, 2024
@prisis
Copy link
Member Author

prisis commented Sep 7, 2024

shini wanted that i split this MR into 2 -> shini did open new prs with the changes.

  • i need to make a change in packem to not normalize the package.json on the fly, (currently changing the code for this)
  • i need to remove ^ from the versions
  • need to move the typesVersions on the correct place

@prisis
Copy link
Member Author

prisis commented Sep 7, 2024

Should i move attw into a new PR? Will move it into a new PR

package.json Outdated Show resolved Hide resolved
@ST-DDT ST-DDT added this to the v9.0 milestone Sep 7, 2024
@ST-DDT ST-DDT added the c: bug Something isn't working label Sep 7, 2024
@prisis prisis changed the title draft: fix: fixed support for node10 and reduced bundle size fix: fixed support for node10 and reduced bundle size Sep 8, 2024
"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
},
"import": {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: IMO this should be default, just to be really safe and we don't forget any unpopular exports (e.g. "node" or "browser")
better they ship incompatible versions but could be worked around somehow, as instead they are not shipped at all

same for line 132

package.json Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

I get the following warning during build:

WARNING [packem] [plugin:patch-types] packem_shared/airline-!{001}.d.cts contains confusing identifier names

  • Casing$1
type Casing = 'upper' | 'lower' | 'mixed';
[...]
type Casing$1 = 'lower' | 'upper' | 'mixed';

@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

I get the following error during playground build:

Syntax Error: Thread Loader (Worker 0)
~\git\Faker\faker\dist\packem_shared\Faker-cIuF47cl.mjs: Static class blocks are not enabled. Please add @babel/plugin-transform-class-static-block to your configuration.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

The build process also edits my package.json:

 "typesVersions": {
    ">=5.0": {
      ".": [
-        "./dist/index.d.ts"
+        "./dist\\index.d.ts"
      ],
      "locale/*": [
-        "./dist/locale/*.d.ts"
+        "./dist\\locale\\*.d.ts"
      ]
    }
  },

@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

Team Decision

We will first merge #3093, but we keep this as an alternative once the remaining issues are resolved.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: infra Changes to our infrastructure or project setup and removed c: bug Something isn't working labels Sep 12, 2024
@ST-DDT ST-DDT modified the milestones: v9.0, vAnytime Sep 12, 2024
@ST-DDT ST-DDT changed the title fix: fixed support for node10 and reduced bundle size infra: replace tsup with packem Sep 12, 2024
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: infra Changes to our infrastructure or project setup needs rebase There is a merge conflict p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[9.0.0] Types are not properly emitted
3 participants