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

docs: Add example for loading ESM from CommonJS #1236

Merged
merged 14 commits into from Aug 28, 2021

Conversation

jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Aug 6, 2021

Purpose

Few ppl started complaining about beta-10 going ESM and are unable to load it. The purpose of this PR is to provide them a alternative method to load ESM modules from CJS

Changes

Have only updated some documents and suggested this way of loading fetch:

// mod.cjs
const fetch = (...args) => import('node-fetch').then(mod => mod.default(...args));

Additional information

There are both pro & cons with ESM.
Bundle all dependencies and having own instances is not desirable. It can make working with different whatwg:streams versions incompatible


  • I prefixed the PR-title with docs:
  • I updated ./docs/CHANGELOG.md with a link to this PR or Issue
  • I updated ./docs/v3-UPGRADE-GUIDE
  • I updated the README.md
  • Should maybe link to a ESM upgrade guide blog/article of how to upgrade a cjs package to esm and how to use different compilers? ...any suggestions? can do this later...

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Neat 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Aug 16, 2021

What to you think about adding this bootstrap approach as a way of loading a ESM package from a complete legacy commonjs codebase?

// bootstrap.js
import('node-fetch').then(({
  default: fetch,
  Headers,
  Request,
  Response
}) => {
  globalThis.fetch = fetch
  globalThis.Headers = Headers
  globalThis.Request = Request
  globalThis.Response = Response
  require('./app.js')
})

Shorter version:

// bootstrap.js
import('node-fetch').then(({
  default: fetch,
  ...rest
}) => {
  Object.assign(globalThis, rest, { fetch })
  require('./app.js')
})

This had me thinking... can you use preload?

-r, --require module#

Preload the specified module at startup.

@Richienb
Copy link
Member

If you're going to assign globals within an asynchronously executed function, then you might as well just await it and get the values that way:

const {
  default: fetch,
  Headers,
  Request,
  Response
} = await import("node-fetch")

This would be best executed within asynchronous functions since CommonJS does not support top-level await.

@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Aug 21, 2021

I where thinking about linking to https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c but it seems highly debatable about good and bad things and includes a few negative comments.
It's a very good guide. I almost wish it was locked and had all some comments removed... i wish it was targeted more at a general scale and didn't had so many comments about Typescript, React vs Javascript comments

There is also a few forks worth mentioning: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c/forks

We also have: https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77 and https://blog.sindresorhus.com/hello-modules-d1010b4e777b

docs/v3-UPGRADE-GUIDE.md Outdated Show resolved Hide resolved
Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Looked thru everything again, looks great 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
docs/CHANGELOG.md Outdated Show resolved Hide resolved
docs/v3-UPGRADE-GUIDE.md Outdated Show resolved Hide resolved
docs/v3-UPGRADE-GUIDE.md Outdated Show resolved Hide resolved
docs/v3-UPGRADE-GUIDE.md Outdated Show resolved Hide resolved
docs/v3-UPGRADE-GUIDE.md Outdated Show resolved Hide resolved
@Richienb Richienb changed the title docs: Example loading ESM from cjs docs: Add example for loading ESM from CommonJS Aug 21, 2021
@jimmywarting
Copy link
Collaborator Author

@Richienb i fixed all your suggestions, please have another look

@ThaUnknown
Copy link

ThaUnknown commented Aug 27, 2021

is 40 lines of documentation what's preventing us from making v3 stable?

@jimmywarting
Copy link
Collaborator Author

jimmywarting commented Aug 27, 2021

is 40 lines of documentation what's preventing us from making v3 stable?

kind of... this is the only thing left in the v3 milestone
ppl have had concerns/problem importing pure ESM package in beta-10. This address those problem + the docs where a bit outdated

Only need 1 more acceptances from one of the maintainers and then we can release/tag v3 as stable


...Feels like some of the maintainers don't have much time or interest anymore that's why updates comes so rarely
Should maybe invite new maintainers

README.md Outdated Show resolved Hide resolved
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.

v3 upgrade Docs needs to be updated
5 participants