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

point pkg.main to cjs modules #3479

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Conversation

charbugs
Copy link
Collaborator

@charbugs charbugs commented Jul 14, 2021

It is common in many npm projects that the "main" field of the package.json points to the CJS modules. Currently we let the main field point to the UMD bundle. In theory this shouldn't be a problem as UMD supports CJS (as well as AMD and the browser). But it's a bit more complicated:

The UMD is built by Webpack; and when Webpack resolves the import paths of the dependencies it will by default use the ES modules of the libraries (if they provide them). Actually it shouldn't make a difference whether Webpack uses CJS or ES modules from a library. But in some libraries the code of ES and CJS differs slightly to work around plattform differences (e.g. Node vs. browser). That is, the UMD bundle may not work in in CJS environments out of the box even if a library provides alternative CJS code that will work.

Here is an example of an issue with the Mirador UMD bundle and Jest

Somewhere in my app:

import Mirador from 'mirador'

export function createMirador() {
  return Mirador.viewer({ id: 'mirador-container' })
}

And the test:

import { createMirador } from './somewhere'

test('renders Mirador', function() {
  const container = document.createElement('div')
  container.id = 'mirador-container'
  document.body.append(container)

  createMirador()
  expect(document.getElementsByClassName('mirador-viewer').length).toBe(1)
})

Currently without the fix of this PR Jest will throw an error saying:

crypto.getRandomValues() not supported. See https://github.com/uuidjs/uuid#getrandomvalues-not-supported
...
at rng (node_modules/mirador/dist/webpack:/Mirador/node_modules/uuid/dist/esm-browser/rng.js:14:13)

The problem here is that Jest is a CJS environment and runs the Mirador UMD bundle that was refered to by pkg.main. The bundle contains code from UUID that uses the browser crypto API which is not supported by jest/jsdom. I think this is not intended by UUID because it also provides code that will work under Jest (see uuid/dist/rng.js).

We can fix that by pointing pkg.main field to the CJS modules of Mirador. That way a CJS environment can decide by itself what kind of modules to import by analyzing the package.json of the libraries.

Do you see any trouble with that? Do you know any tool that expects a UMD bundle at pkg.main rather than a CJS module? Could that change break any integration?

@codecov-commenter
Copy link

Codecov Report

Merging #3479 (d15014c) into master (c941b70) will not change coverage.
The diff coverage is n/a.

❗ Current head d15014c differs from pull request most recent head 0fd531b. Consider uploading reports for the commit 0fd531b to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3479   +/-   ##
=======================================
  Coverage   88.91%   88.91%           
=======================================
  Files         198      198           
  Lines        3409     3409           
=======================================
  Hits         3031     3031           
  Misses        378      378           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c941b70...0fd531b. Read the comment docs.

@charbugs charbugs requested a review from cbeer July 15, 2021 20:01
@charbugs
Copy link
Collaborator Author

@cbeer Sorry, I re-requested you for a review by accident.

@cbeer
Copy link
Collaborator

cbeer commented Jul 15, 2021

Still looks good to me, just waiting to see if anyone else can answer your questions 🤷

@cbeer cbeer merged commit f144742 into master Aug 2, 2021
@cbeer cbeer deleted the point-pkg-main-to-cjs-modules branch August 2, 2021 14:16
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.

3 participants