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

fix: use require/import export map in package.json #697

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

revmischa
Copy link
Contributor

Closes #696 - please see there for more details

This is to make the export map compliant with NodeJS documentation and recommendations: https://nodejs.org/api/packages.html#conditional-exports

I believe the description and example there is pretty clear. It also mentions that using node as a mapping is usually wrong:

"node" - matches for any Node.js environment. Can be a CommonJS or ES module file. In most cases explicitly calling out the Node.js platform is not necessary.

@revmischa revmischa requested a review from a team as a code owner March 27, 2022 16:12
@revmischa revmischa changed the title Use require/import export map in package.json fix: Use require/import export map in package.json Mar 27, 2022
@codecov
Copy link

codecov bot commented Mar 27, 2022

Codecov Report

Merging #697 (393926d) into main (5ed963f) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 393926d differs from pull request most recent head 1d641c6. Consider uploading reports for the commit 1d641c6 to get more accurate results

@@           Coverage Diff           @@
##             main     #697   +/-   ##
=======================================
  Coverage   99.34%   99.34%           
=======================================
  Files        1923     1923           
  Lines      176903   176887   -16     
  Branches      900      897    -3     
=======================================
- Hits       175742   175732   -10     
+ Misses       1105     1099    -6     
  Partials       56       56           
Impacted Files Coverage Δ
src/index.ts 94.87% <0.00%> (-2.57%) ⬇️
src/random.ts 99.44% <0.00%> (-0.01%) ⬇️
src/fake.ts 100.00% <0.00%> (ø)
src/faker.ts 100.00% <0.00%> (ø)
src/lorem.ts 100.00% <0.00%> (ø)
src/system.ts 96.48% <0.00%> (+0.03%) ⬆️
src/vendor/user-agent.ts 98.03% <0.00%> (+0.28%) ⬆️
src/name.ts 100.00% <0.00%> (+0.30%) ⬆️
src/finance.ts 100.00% <0.00%> (+0.69%) ⬆️
src/git.ts 100.00% <0.00%> (+1.69%) ⬆️

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

This does not work right now 🤔

I get

file:///home/shinigami/OpenSource/faker.js-tests/esm/index.js:2
import { faker } from "@faker-js/faker";
         ^^^^^
SyntaxError: Named export 'faker' not found. The requested module '@faker-js/faker' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@faker-js/faker';
const { faker } = pkg;

Please checkout the https://github.com/faker-js/playground (and comment out the testings for #642)

@import-brain import-brain added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs labels Mar 27, 2022
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Tested locally with the playground, it's working 👍

@Shinigami92 Shinigami92 added the s: accepted Accepted feature / Confirmed bug label Mar 27, 2022
@Shinigami92 Shinigami92 requested review from a team March 27, 2022 17:25
@Shinigami92 Shinigami92 added this to the v6.1 - First bugfixes milestone Mar 27, 2022
@Shinigami92
Copy link
Member

Please unlock your repo / this PR so maintainers can make changes / rebase the main branch into it.
We wanted to release this in v6.1.0 but now we are stuck and need to wait on you 🙁

@Shinigami92 Shinigami92 changed the title fix: Use require/import export map in package.json fix: use require/import export map in package.json Mar 28, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) March 28, 2022 17:01
@Shinigami92 Shinigami92 merged commit 0f74908 into faker-js:main Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node w/ ESM uses CJS files
4 participants