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

feat: add support for native ESM imports, make repository ESM-first #2409

Merged
merged 19 commits into from
Jan 18, 2024

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Oct 9, 2023

Overview

Fixes #2068

Converts the repository to be ESM first. Ensures that every package published has "type": "module" and set "exports". As a natural consequence, tools like ESLint, Jest are running in ESM mode too.

With these changes, I'm able to import and use react-pdf in ESM Node.js projects, and build it with esbuild too.

Details

Setting "type": "module" and "exports"

Before this PR, package.jsons contained main and module entires. The former is universal, the latter - kinda-supported-by-most-bundlers-maybe way to point them to ESM modules. But this does not work with Node.js. To make Node.js aware of the ESM version, "type": "module" and "exports" must be set.

Note: We may want to push this further by introducing .cjs extensions, or use /dist/cjs and /dist/esm directories, each with their own package.json with "type": "module"/"commonjs".

Adding file extensions in select places

There were 4 files that appear to be running in Node without bundling or any other help that would handle extensionless module resolution. For the whole repository to work with Node.js in ESM mode, I had to add .js to 6 imports.

Note: We may want to push this further by adding all required extensions everywhere. This PR includes the bare minimum to make react-pdf work with ESM imports. In modern Node.js, you are actually required to include extensions in your imports. However, this would mean I would need to change hundreds of files at once, which would make it unlikely for this PR to ever get merged. It's quite large anyway...

Configuration

Due to Jest changes (see below), jest/globals were removed from ESLint env config. Because of @jest/globals import, an exemption from import/no-extraneous-dependencies was also added for unit tests.

Tooling

Babel

By setting "type": "module", module.exports no longer worked in babel.config.js. This had to be changed to export default.

ESLint

Similarly to Babel, module.exports was no longer an option. However, ESLint in your repository didn't want to play nicely with ESM either, so I opted for pure JSON instead.

Jest

By far most significant change in this PR.

  • Jest doesn't play nicely with ESM modules; support for it is experimental. It also required me to change the command from just jest to yarn node --experimental-vm-modules \"$(yarn bin jest)\".
  • Some unit tests were using require() syntax, which didn't work anymore.
  • In ESM mode, jest global is not available, to jest had to be explicitly imported from @jest/globals instead.
  • In ESM mode, __dirname is not available, so path.dirname(url.fileURLToPath(import.meta.url)) "polyfill" was used instead.

Additional notes

Note: I was forced to add updated node-gyp to our devDependencies for the project to even install correctly, in 4c0fbcd. I may split it into a separate PR if you want.

Note: While I'm quite confident this setup works very well in ESM Node.js, CJS was not verified in any way. It would be a good idea to test it out in legacy Node.js environment before merging.

Note: The PR is currently failing due to an issue in external dependency, browserify-zlib. I have raised a PR to fix it: browserify/browserify-zlib#45

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2023

🦋 Changeset detected

Latest commit: cb13b1c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@react-pdf/primitives Minor
@react-pdf/stylesheet Minor
@react-pdf/examples Minor
@react-pdf/renderer Minor
@react-pdf/textkit Minor
@react-pdf/layout Minor
@react-pdf/pdfkit Minor
@react-pdf/png-js Minor
@react-pdf/render Minor
@react-pdf/svgkit Minor
@react-pdf/image Minor
@react-pdf/font Minor
@react-pdf/fns Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wojtekmaj wojtekmaj changed the title Convert repository to ESM Add support for native ESM imports, make repository ESM-first Oct 9, 2023
"main": "./lib/react-pdf.cjs.js",
"module": "./lib/react-pdf.es.js",
"exports": {
".": {
"import": "./lib/react-pdf.es.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we also need to add types there, as types are not siblings of the outputted JS files.

Copy link
Owner

@diegomura diegomura Jan 15, 2024

Choose a reason for hiding this comment

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

How would this affect backwards compatibility? I know some module resolvers do not work with exports yet right? @wojtekmaj

@diegomura
Copy link
Owner

@wojtekmaj would be great to add this. Can you rebase? 🙏🏻

@wojtekmaj
Copy link
Contributor Author

@diegomura There you go!

@wojtekmaj

This comment was marked as outdated.

@wojtekmaj
Copy link
Contributor Author

@diegomura Resolved Jest issue, but I'm not entirely happy with it (of course, since Jest can't handle ESM properly). However, I'll have Vitest migration ready to go for you as soon as this one gets merged :D

@diegomura
Copy link
Owner

Thanks @wojtekmaj ! Can't check in full detail now but I'll soon. In the meantime, do you mind explaining the benefits of moving in this direction? I get the benefits of ESM but my worry is this will cause issues for lots of consumers using old bundlers or node versions.

@wojtekmaj
Copy link
Contributor Author

wojtekmaj commented Jan 15, 2024

Absolutely! My tree of thoughts on why we should move towards ESM-first.

  • This resolves the biggest issue: @react-pdf/renderer is literally unusable in Node.js projects using ESM right now. Well, I was able to pull it off, but I'm using like 15 custom patches to make it work. It's nothing but pain in the ass.
  • This does not remove any configuration that was meant for older bundlers/Node.js versions, only adds standardized "exports" entry to resolve ESM files, on top of non-standard "module".
    • Newer bundlers/Node.js versions will be able to use "exports"
    • Older bundlers/Node.js versions will simply ignore the new entry
  • This does not remove CJS lib code. It should continue to work just fine, unless someone used undocumented import paths.
  • At the moment, module resolution is kinda broken everywhere.
  • In the recent Node version, there's an experimental flag that flips the default from CJS to ESM for packages without type defined in package.json. I don't think it's going to be the default anytime soon, but it's a clear sign that the trend continues.
  • ESM is unquestionably better especially for usage in browsers.
    • Will require more work, but eventually we know may be able to ship react-pdf as pure ESM that works in the browser without bundling. Would be cool!
  • Migration will make it easier to migrate from Jest to Vitest.

Copy link
Owner

@diegomura diegomura left a comment

Choose a reason for hiding this comment

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

utACK

All look good! Left some small questions to answer before I merge but I'll merge immediately after. Thanks for the work @wojtekmaj

package.json Outdated
@@ -61,12 +62,14 @@
"jest-image-snapshot": "^6.1.0",
"lerna": "^8.0.2",
"lint-staged": "^10.5.4",
"node-gyp": "^10.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

@wojtekmaj wojtekmaj Jan 18, 2024

Choose a reason for hiding this comment

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

Hmmm, works without it directly declared, but please mind, it's gonna stay in our deps anyway. I can see it was added in #2478.


import setAlignSelf from '../../src/node/setAlignSelf';

// yoga-layout sets default export using non-standard __esModule property, so we need to
// make an additional check in case it's used in a bundler that does not support it.
const Yoga = 'default' in yogaModule ? yogaModule.default : yogaModule;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we extract this in a layout/yoga.js file? We can capture it and I can do it separately. It might not be necessary either as I'd like to experiment with async yoga which I think it's more performant and uses wasm build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, defining that 35 times was idiotic :D Fixed that.

I don't think move to wasm will change anything here though.

import { basename, extname } from 'path';
import { parse } from '../afm';
// This file is ran directly with Node - needs to have .js extension
Copy link
Owner

Choose a reason for hiding this comment

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

Don't we always need .js extension with ESM?

Copy link
Contributor Author

@wojtekmaj wojtekmaj Jan 18, 2024

Choose a reason for hiding this comment

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

Yes and no! The key word in this comment is "directly".

In most cases, react-pdf is pre-bundled. When bundled, module resolution is already done by the bundler and these rules no longer apply.

I WOULD do this if I were you sooner or later, see "Adding file extensions in select places" in original post for rationale.

@@ -3,26 +3,33 @@
"version": "3.1.17",
"license": "MIT",
"description": "Create PDF files on the browser and server",
"types": "index.d.ts",
Copy link
Owner

Choose a reason for hiding this comment

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

No longer needed?

Copy link
Contributor Author

@wojtekmaj wojtekmaj Jan 18, 2024

Choose a reason for hiding this comment

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

Not only no longer needed, but actively harmful for type resolution in TypeScript. Instead, each bundled .js file gets its own .d.ts sibling file. This makes the bundle larger, unfortunately, but this is the reason we'll be seeing a vast improvement on arethetypeswrong after this PR.

Read more: https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md

@diegomura diegomura changed the title Add support for native ESM imports, make repository ESM-first feat: add support for native ESM imports, make repository ESM-first Jan 17, 2024
@diegomura
Copy link
Owner

Older bundlers/Node.js versions will simply ignore the new entry

This is good to hear. I was thinking if would be convenient to otherwise do a major bump. Recent yoga-layout bump included library using exports, and I already got lots of complaints about envs breaking. But does not seem to be necessary then

@wojtekmaj
Copy link
Contributor Author

This is good to hear. I was thinking if would be convenient to otherwise do a major bump. Recent yoga-layout bump included library using exports, and I already got lots of complaints about envs breaking. But does not seem to be necessary then

I genuinely believe we can do this without the major bump. I successfully migrated more than 30 OSS repositories of my own to ESM-first in a minor bump and received 0 complaints. Granted, they were much less complex than react-pdf, but I now know a thing or two, and worry not - if anything goes south, I'll have your back while bugfixing :)

@diegomura diegomura merged commit b6a14fd into diegomura:master Jan 18, 2024
1 check passed
@diegomura
Copy link
Owner

Thanks for addressing all my questions and the work done here!

@wojtekmaj wojtekmaj deleted the esm branch January 18, 2024 09:46
mskec pushed a commit to mskec/react-pdf that referenced this pull request Feb 26, 2024
…iegomura#2409)

* Set "type": "module" and add "exports"

* Convert Babel config to ESM

* Add our own node-gyp so that canvas can be built

* Fix parse:afm command, remove useless babel-node

* Convert Jest config and CJS Jest tests to ESM

* Convert ESLint config to JSON

* Allow extraneous dependencies in test files

* Change .size-limit config extension to .cjs

* Fix build

* Add changeset

* Add types

* Fix faux ESM yoga-layout import

* Make ESLint happy

* Remove unnecessary eslint-disable

* Fix broken imports in browserify-zlib

See browserify/browserify-zlib#45

* Fix Jest command?

* Use .cjs and .js instead of .cjs.js and .es.js respectively, copy types for each outputted file separately

* Remove direct node-gyp dependency

* Move Yoga hack to a separate file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node esm imports not working correctly
2 participants