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

Package data for NPM as ESM and CJS #12169

Merged
merged 6 commits into from
Apr 7, 2022
Merged

Conversation

lucacasonato
Copy link
Collaborator

Previously only a CJS entrypoint was provided. This provides a ESM
entrypoint that can be directly used from Deno or a browser.

Previously only a CJS entrypoint was provided. This provides a ESM
entrypoint that can be directly used from Deno or a browser.
@github-actions github-actions bot added the scripts 📜 Issues or pull requests regarding the scripts in scripts/. label Aug 24, 2021
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Thank you for splitting this out, I'm super excited for this! 😄 I have just a small suggestion and a question, but from a quick skim of the code this is looking good to me overall!

scripts/release-build.js Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
scripts/release-build.js Outdated Show resolved Hide resolved
scripts/release-build.js Outdated Show resolved Hide resolved
@queengooborg
Copy link
Collaborator

Okay, so I had a feeling this might be a breaking change, and it looks like my suspicions were correct. BCD's exported packages don't expose the version number, so I require the included package.json and use that instead. By adding the exports key to the package.json, I wouldn't be able to require any file I wanted from the package, so my program would break. We should be sure to bump the major semver when this is launched!

@lucacasonato
Copy link
Collaborator Author

Import assertions are now widely available in server side runtimes. Chrome supports them, and so do both Deno (since 1.17) and Node (since v17). As such we could now keep the JSON in a seperate file for publishing again, and then re-export it from the ESM module using import assertions.

@queengooborg We could do the same with the version from package.json.

@lucacasonato
Copy link
Collaborator Author

Actually - scrap that. Node 17 does not support JSON modules without the --experimental-json-modules flag... :/

I guess the current thing here is the best thing we could do for now.

@lucacasonato
Copy link
Collaborator Author

Node unflagged import assertions in v17.5.0 (released 2022-02-10). Probably too soon to rely on them?

@queengooborg
Copy link
Collaborator

Yeah, I'd say it's too soon, especially since our developer requirement is NodeJS v14. Thanks for double-checking!

@lucacasonato
Copy link
Collaborator Author

Makes sense. Once the requirement is NodeJS v18, this is the patch to apply to clean up:

From: Luca Casonato <[email protected]>
Date: Thu, 7 Apr 2022 23:29:06 +0200
Subject: [PATCH] Use import assertions to import data in ESM

---
 scripts/release-build.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/release-build.js b/scripts/release-build.js
index eaba25e7e..fd2a20a9c 100644
--- a/scripts/release-build.js
+++ b/scripts/release-build.js
@@ -20,9 +20,9 @@ async function writeData(data) {
   await fs.writeFile(dest, data);
 }
 
-async function writeIndexESM(data) {
+async function writeIndexESM() {
   const dest = path.resolve(directory, 'index.js');
-  const content = `export default ${data};\n`;
+  const content = `import data from "./data.json" assert { type: "json" };\nexport default data;\n`;
   await fs.writeFile(dest, content);
 }
 
@@ -91,7 +91,7 @@ async function main() {
 
   await writeManifest();
   await writeData(data);
-  await writeIndexESM(data);
+  await writeIndexESM();
   await copyFiles();
 
   console.log('Data bundle is ready');
-- 
2.35.1

@github-actions github-actions bot added the docs ✍️ Issues or pull requests regarding the documentation of this project. label Apr 7, 2022
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

I went ahead and updated the readme to include an ESM import example, but all in all this is looking great to me! Thank you so much for this, I'm super excited to see this in the next release!

@queengooborg queengooborg merged commit b740c60 into mdn:main Apr 7, 2022
@lucacasonato lucacasonato deleted the public_api_esm branch April 7, 2022 22:42
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Apr 12, 2022
This reverts commit b740c60.

This change is semver major and I'm not prepared to do that this week.
ddbeck added a commit that referenced this pull request Apr 12, 2022
This reverts commit b740c60.

This change is semver major and I'm not prepared to do that this week.
ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Apr 12, 2022
Revert "Revert "Package data for NPM as ESM and CJS (mdn#12169)" (mdn#15774)"

This reverts commit 6f3a8b7.
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/.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants