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

add CJS bundle highlight.js/compat #3

Open
wants to merge 1 commit into
base: esm-tests-more-work
Choose a base branch
from

Conversation

aduh95
Copy link

@aduh95 aduh95 commented Mar 22, 2021

Refs: #2 (comment)

Changes

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

@@ -31,6 +31,12 @@ async function buildNodeLanguage(language) {
await rollupWrite(input, output);
}

async function buildNodeBundle() {
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't buildNodeLanguage also have to be updated/suplimented to build all the individual languages in CJS?

Copy link
Author

Choose a reason for hiding this comment

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

It depends, we could decide that we supply only a bundle CJS file – or we can go the dual module package way and provide a CJS module for each language, but I thought you said that was not acceptable?

@joshgoebel
Copy link
Owner

joshgoebel commented Mar 23, 2021

Can you not use type:module with conditional exports? I'm trying:

  7   "exports": {
  6     ".": {
  5       "require": "./compat.cjs",
  4       "import": "./lib/index.js"
  3     },
  2     "./lib/index": {
  1       "require": "./compat.cjs",
44        "import": "./lib/index.js"
  1     }
  2   },

But still require "./build" blows up saying I can't require an ESM module.

@aduh95
Copy link
Author

aduh95 commented Mar 23, 2021

Conditional exports work whether or not you set a "type" field in your package.json; however, the export map is used only when you require/import the package by name – so require("./build") will not use the export map because ./build is a directory path, not a package name.

To test the conditional exports, the "right thing to do" is to use a self-reference to highlight.js: https://nodejs.org/api/packages.html#packages_self_referencing_a_package_using_its_name

// root package.json
{
  "name": "highlight.js",
  "private": true,
  "exports": {
       ".": {
         "require": "./build/compat.cjs",
         "import": "./build/lib/index.js"
       },
       "./lib/index": {
         "require": "./build/compat.cjs",
         "import": "./build/lib/index.js"
       },
       "./lib/languages/*": "./build/lib/languages/*.js"
     }
}
// build/package.json
{
  "name": "highlight.js",
  "type": "module",
  "exports": {
       ".": {
         "require": "./compat.cjs",
         "import": "./lib/index.js"
       },
       "./lib/index": {
         "require": "./compat.cjs",
         "import": "./lib/index.js"
       },
       "./lib/languages/*": "./lib/languages/*.js"
     }
}

Hopefully that makes sense, let me know if that's not clear.

@joshgoebel
Copy link
Owner

joshgoebel commented Mar 23, 2021

Ok that works... but... Ugh, there has got to be some better way to test the built package asset independently of its source...

Because this doesn't depend on the build package.json at all. I never even modified it and this still works, so it's not really testing anything at all other than the source package.json.

How is one supposed to properly test conditional requires of a fully built package in a CI environment, etc?

I feel like we need a way to install the newly built package itself via npm and then run tests (or binary stubs) against that by name... am I thinking wrong here?

@aduh95
Copy link
Author

aduh95 commented Mar 23, 2021

I feel like we need a way to install the newly built package itself via npm and then run tests (or binary stubs) against that by name... am I thinking wrong here?

Yep, that's probably necessary as an integration test to run in the CI. For local testing, having the root package.json linking to the build folder is probably enough though.

@joshgoebel
Copy link
Owner

joshgoebel commented Mar 23, 2021

It seems you can do this with npm install directory:

npm init
npm install ../highlight.js/build/
# run tests in the scope of your new package (by name)

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.

2 participants