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

Allow ESM imports of the data #16232

Merged
merged 12 commits into from
May 18, 2022
Merged

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented May 11, 2022

This PR fixes #15916 and allows ESM users to directly import BCD by performing two things:

  • Remove the redundant index.js to import the data (NodeJS has always been able to require() JSON) and export data.json by default
  • Add a wrapper for NodeJS v12-v14 ESM, since they do not support import assertions, which can be imported via import bcd from "@mdn/browser-compat-data/NodeWrapper";

I tested to make sure this approach works using require() in NodeJS v18, v16, v14, v12, and even v4; and using import in NodeJS v18, v16, v14, and v12, all successfully importing BCD.

This is marked as a semver-major-bump because exports is defined which may break existing setups (i.e. if users are already trying to import data.json directly).

This closes #15775, which also offers ESM import capability, but at the cost of double the bundle size.

@github-actions github-actions bot added the scripts 📜 Issues or pull requests regarding the scripts in scripts/. label May 11, 2022
@queengooborg queengooborg added needs-release-note 📰 semver-minor-bump ➕ A change that adds a new, non-potentially-breaking feature for consumers labels May 11, 2022
@queengooborg queengooborg requested a review from foolip May 11, 2022 16:31
@github-actions
Copy link

This pull request has merge conflicts that must be resolved before we can merge this.

1 similar comment
@github-actions
Copy link

This pull request has merge conflicts that must be resolved before we can merge this.

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before we can merge this.

@queengooborg queengooborg requested a review from caugner May 13, 2022 22:16
@queengooborg queengooborg added semver-major-bump 🚨 A change that is potentially breaking for consumers and removed semver-minor-bump ➕ A change that adds a new, non-potentially-breaking feature for consumers labels May 13, 2022
This was referenced May 17, 2022
@queengooborg queengooborg changed the title Simplify release bundle by removing index.js Allow ESM imports of the data May 17, 2022
@github-actions github-actions bot added the docs ✍️ Issues or pull requests regarding the documentation of this project. label May 17, 2022
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just some bike-shedding

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/release/build.js Outdated Show resolved Hide resolved
scripts/release/build.js Outdated Show resolved Hide resolved
scripts/release/build.js Show resolved Hide resolved
@queengooborg queengooborg merged commit afb358e into mdn:main May 18, 2022
@queengooborg queengooborg deleted the release-bundle branch May 18, 2022 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs ✍️ Issues or pull requests regarding the documentation of this project. scripts 📜 Issues or pull requests regarding the scripts in scripts/. semver-major-bump 🚨 A change that is potentially breaking for consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish ESM-ready bundles of BCD
2 participants