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

(chore) dual package #3188

Merged
merged 2 commits into from May 30, 2021
Merged

(chore) dual package #3188

merged 2 commits into from May 30, 2021

Conversation

joshgoebel
Copy link
Member

@joshgoebel joshgoebel requested a review from allejo May 14, 2021 20:27
@joshgoebel joshgoebel modified the milestones: 11.0, 11.0-beta1 May 14, 2021
@joshgoebel
Copy link
Member Author

Look right? @aduh95

@aduh95
Copy link
Contributor

aduh95 commented May 18, 2021

Thanks for the ping!
For my use-case, the implementation in this PR is a regression: if that's the way highlight.js is going, the situation is the same as with v10: the browser doesn't understand CJS modules, so I can't load the core module from my browser without a bundler.

Let me see if I understand what's the issue this PR is trying to solve. In the Node.js docs, it says:

If the package main export is a constructor, an instanceof comparison of instances created by the two versions returns false

I don't think we expose a class, so I suppose it's a non-issue.

if the export is an object, properties added to one (like pkgInstance.foo = 3) are not present on the other.

So that means the user sets debugMode on a CJS require, it won't apply to an instance loaded with import – and same with any configuration. I can see that could be an issue for Node.js users, but a non-issue for browser or bundler users. Is my understanding correct?

Assuming I understand this issue correctly, I think another way of solving it would to let the ESM versions untouched, and add a "node" entry in the package exports like this:

    ".": {
      "node": "./lib/index.js",
      "require": "./lib/index.js",
      "import": "./es/index.js"
    },
    "./package.json": "./package.json",
    "./lib/common": {
      "node": "./lib/index.js",
      "require": "./lib/common.js",
      "import": "./es/common.js"
    },
    "./lib/core": {
      "node": "./lib/core.js",
      "require": "./lib/core.js",
      "import": "./es/core.js"
    },

What do you think? I can try to drop a PR if that helps.

@joshgoebel
Copy link
Member Author

For my use-case, the implementation in this PR is a regression:

I think you've always been riding the coattails of a use case we really don't care directly about (yet). 🙂 Honestly ESM browser modules sound like they might more properly belong in our CDN build. I don't think I've quite wrapped my head around "one standard to rule them all" yet... many users aren't on this bandwagon and since we already have a Node.js NPM package and CDN package if there was a ESM browser build I think it may belong in the CDN package. Thoughts?

We export an instance of HLJS which means if someone uses two different libraries and one imports us and another requires us that now there will be TWO Highlight.js monoliths, with all different settings, languages, debug modes, etc... this can cause all sorts of weirdness and confusion. To solve this the ESM entry point has to literally require the CJS version - so that this is solved by the require cache - now there is only one HLJS, the one required.

This is specifically the solution the docs seem to recommend to avoid there being two instances of our library. Yes, my understanding is it's a Node issue.

and add a "node" entry in the package exports like this:

Ugh. So that pretty much means no ESM for Node... and we'd only be including the ESM for the browser?

@aduh95
Copy link
Contributor

aduh95 commented May 19, 2021

Ugh. So that pretty much means no ESM for Node... and we'd only be including the ESM for the browser?

Is it very different for what this PR is doing? What's the difference between pointing straight to the CJS file or having a a ES modules that does nothing but re-export the CJS module?

Correct me if I'm wrong, but only core.js is subject to the dual package hazard, right? index and core could be kept as ESM pointing to the highlight.js/lib/core:

    ".": {
      "require": "./lib/index.js",
      "import": "./es/index.js"
    },
    "./package.json": "./package.json",
    "./lib/common": {
      "require": "./lib/common.js",
      "import": "./es/common.js"
    },
    "./lib/core": {
      "node": "./lib/core.js",
      "require": "./lib/core.js",
      "import": "./es/core.js"
    },

I think you've always been riding the coattails of a use case we really don't care directly about (yet). 🙂 Honestly ESM browser modules sound like they might more properly belong in our CDN build. I don't think I've quite wrapped my head around "one standard to rule them all" yet... many users aren't on this bandwagon and since we already have a Node.js NPM package and CDN package if there was a ESM browser build I think it may belong in the CDN package. Thoughts?

I think that's fair, I understand my use-case is a bit peculiar and would understand if the project decides not to support it. Another use-case which may be more spread out, there's also bundler users who can benefit from having an ESM-only path (E.G.: when using Rollup, you can drop @rollup/plugin-commonjs from your deps and get a smaller bundle).

@joshgoebel
Copy link
Member Author

Correct me if I'm wrong, but only core.js is subject to the dual package hazard, right? index and core could be kept as ESM pointing to the highlight.js/lib/core:

I'm not sure... anything that a user might reasonably require/export that returns our "global" instance would be problematic... but it might be possible to only stub core... I was just going with the simpler build pipeline.


A feel a larger problem here is that our highlight.js NPM library is meant for two things:

  • Use on the server-side with Node.js
  • Bundling

It's not meant for "raw" use... the code is not compressed, it's not minified, it's not "web-ready". You could use it as is, but I don't think that's something we should encourage. In contrast though, we already ship a web ready package: cdn-assets.

I think the discussion we should perhaps be having (right now) is about adding ESM libs to our CDN build and related considerations. Perhaps one day there will be "one package to rule them all" but I don't think we're there yet. Sounds like the kind of thing worth discussing when v12 rolls around.

Is there a reason that you couldn't just add @highlightjs/cdn-assets to your project instead of highlight.js if it included minified ESM builds?

@joshgoebel
Copy link
Member Author

Out of curiosity how are you intending to get the files from NPM to the browser without a bundler? Are you use using unpkg, CDN, etc?

@aduh95
Copy link
Contributor

aduh95 commented May 19, 2021

Is there a reason that you couldn't just add @highlightjs/cdn-assets to your project instead of highlight.js if it included minified ESM builds?

But it doesn't include ESM build, does it? It looks like instead it's using hljs on the global proxy (I've checked https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.0.0-beta1/build/languages/javascript.min.js, maybe that's not what you are referring to?)

It it did include it, sure I'd use it, but.. why not use the NPM package? As I said in my previous comment, bundler users also benefit from having ESM available.

Out of curiosity how are you intending to get the files from NPM to the browser without a bundler? Are you use using unpkg, CDN, etc?

import highlight from './node_modules/highlight.js/lib/core.js';

@joshgoebel
Copy link
Member Author

But it doesn't include ESM build, does it?

Not currently, I did say "if"...

It it did include it, sure I'd use it, but.. why not use the NPM package? As I said in my previous comment, bundler users also benefit from having ESM available.

  • We historically have always shipped a "web ready" package and I'm not interested in changing that policy (or adding confusion about which package to use) so late in the release process.
  • It's not web ready, so we don't want to encourage people just copying those raw files.
  • The benefit when bundling isn't significant and I still don't understand the pros/cons of node or the benefits/downsides of trying to mix all use cases into a single package.

Technically someone wanting "pure esm" could use the CDN assets version I suppose... UGH. What a pain. I really just don't have any more bandwidth for this.

import highlight from './node_modules/highlight.js/lib/core.js';

The still makes me think you're bundling... how does that actually get on the web server? Are you just copying ALL of node_modules blindly?

@joshgoebel
Copy link
Member Author

joshgoebel commented May 19, 2021

Bottom line:

We already have a ton of infrastructure, history, documentation around making our web builds available via CDN (and secondarily) via NPM... if we do have an ESM library that is intended for use on the web then it should be built with cdn and packaged along with the CJS version in our CDN distribution. It would be very strange for it to not be included there. That may require some additional work/thought... so if you want to have that discussion...

This is irregardless of what happens or doesn't happen with the highlight.js NPM package in the future. (until perhaps one distant day if they should merge somehow)

@aduh95
Copy link
Contributor

aduh95 commented May 19, 2021

Let me say that I really appreciate the time you put into this and the consideration you gave to my request, no matter if it ends up in the v11.x release or not! I'm really sorry if I'm giving you more work, and I understand if you are not interested to invest more time into it.
I've opened #3200 which implements my proposal, but again, no hard feeling if you want to dismiss it to focus on more important topics :)

@joshgoebel
Copy link
Member Author

The problem is now that I've given it thought: ESM builds intended for directly use as-is on the web (your stated use case) should be in our CDN build - that's where people who aren't bundling look to find our web assets... that makes this patch (which I still don't fully understand) much less interesting. Your use case should instead by solved by just adding pure ESM modules to our web CDN distribution. Hence #3199. And if that should require breaking no APIs then that could be done at any time in the 11 cycle.

@aduh95
Copy link
Contributor

aduh95 commented May 19, 2021

I think I agree, if there's a web focus dist which can work with ESM, there's no need to discuss compatibility with the npm package. I just need to remember to use the cdn-assets NPM package when I want to work offline.

The still makes me think you're bundling... how does that actually get on the web server? Are you just copying ALL of node_modules blindly?

tbf I'd do that only when starting new projects (I.E. before I try to add a bundler/minifier build-chain), or for toy-projects for which I don't bother optimising in any way (typically I would transform to unpkg URLs before pushing to prod though, if I ever go as far).

@joshgoebel joshgoebel merged commit 5dc5176 into highlightjs:main May 30, 2021
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.

None yet

2 participants