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: convert to esm #2261

Merged
merged 11 commits into from
Feb 18, 2024
Merged

chore: convert to esm #2261

merged 11 commits into from
Feb 18, 2024

Conversation

Shinigami92
Copy link
Member

Convert the whole faker code base to ESM

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jul 17, 2023
@Shinigami92 Shinigami92 self-assigned this Jul 17, 2023
@Shinigami92 Shinigami92 added the do NOT merge yet Do not merge this PR into the target branch yet label Jul 17, 2023
scripts/bundle.ts Outdated Show resolved Hide resolved
@matthewmayer
Copy link
Contributor

What are the benefits of this?

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (db88a15) 99.56% compared to head (1531c25) 99.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2261      +/-   ##
==========================================
- Coverage   99.56%   99.55%   -0.01%     
==========================================
  Files        2817     2817              
  Lines      251199   251199              
  Branches     1127     1125       -2     
==========================================
- Hits       250100   250088      -12     
+ Misses       1099     1082      -17     
- Partials        0       29      +29     

see 30 files with indirect coverage changes

@Shinigami92
Copy link
Member Author

What are the benefits of this?

support for future features, top-level await in jsdoc generation scripts, enhance speed of disk access, and more

It's a try for #2260 (comment)

@Shinigami92
Copy link
Member Author

After already sinking in around 5 hours of net time, I will give this a last chance today (when I find the time)

I read an article about placing package.json files into the dist/esm and dist/cjs folders that have separate configured "type" fields.
That way the cjs folder would read resolved .js files as cjs instead of esm

If that does not work, I will give up for now and we might even consider to release a @faker-js/faker together with a @faker-js/faker-cjs 🤷
This would even come with some advantages like we can see in the weekly download stats how necessary the cjs version even is and also reduce the unpacked size of each version into half/half

@Shinigami92 Shinigami92 force-pushed the convert-to-esm branch 3 times, most recently from fbbef7e to ca95460 Compare September 11, 2023 17:44
@Shinigami92
Copy link
Member Author

Putting a package.json with { "type": "commonjs" } doesn't work, as not the package.json from the source, but the one next to dist is relevant

@Shinigami92 Shinigami92 added this to the v9 - Next major milestone Sep 13, 2023
@ST-DDT ST-DDT modified the milestones: v9.x, v9.0 Oct 3, 2023
@ST-DDT ST-DDT removed the do NOT merge yet Do not merge this PR into the target branch yet label Feb 8, 2024
@Shinigami92
Copy link
Member Author

awaiting merge of #2391

@Shinigami92 Shinigami92 added s: on hold Blocked by something or frozen to avoid conflicts and removed s: on hold Blocked by something or frozen to avoid conflicts labels Feb 9, 2024
@Shinigami92 Shinigami92 requested a review from ST-DDT February 16, 2024 16:43
@Shinigami92 Shinigami92 marked this pull request as ready for review February 16, 2024 16:43
@Shinigami92 Shinigami92 requested a review from a team as a code owner February 16, 2024 16:43
@Shinigami92 Shinigami92 marked this pull request as draft February 16, 2024 16:43
@Shinigami92
Copy link
Member Author

There are errors 😕
I ran out of time right now, need to look for them later

@Shinigami92 Shinigami92 marked this pull request as ready for review February 16, 2024 17:02
pnpm-lock.yaml Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 requested a review from ST-DDT February 16, 2024 17:11
@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2024

Looks like you have to change how https://github.com/faker-js/faker/blob/next/.github/workflows/commentCodeGeneration.ts is converted to js.
(Or run it using tsx instead?)

@Shinigami92 Shinigami92 marked this pull request as draft February 16, 2024 17:15
@ST-DDT
Copy link
Member

ST-DDT commented Feb 16, 2024

I assume the issue is more in the calling of the file:

const script = require('${{ github.workspace }}/.github/workflows/commentCodeGeneration.js')

@Shinigami92 Shinigami92 marked this pull request as ready for review February 16, 2024 19:49
@ST-DDT ST-DDT requested review from a team February 16, 2024 20:05
@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Feb 16, 2024
@Shinigami92 Shinigami92 merged commit ec5609b into next Feb 18, 2024
17 checks passed
@Shinigami92 Shinigami92 deleted the convert-to-esm branch February 18, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants