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

ESM build does not export named exports #3295

Closed
benmccann opened this issue Jul 31, 2021 · 19 comments · Fixed by #3307
Closed

ESM build does not export named exports #3295

benmccann opened this issue Jul 31, 2021 · 19 comments · Fixed by #3307
Labels
bug help welcome Could use help from community parser
Milestone

Comments

@benmccann
Copy link

Describe the issue/behavior that seems buggy

The ESM build does not export named exports.

The highlight code links to the Node docs on how to setup an ESM build:

// https://nodejs.org/api/packages.html#packages_writing_dual_packages_while_avoiding_or_minimizing_hazards

However, it does not appear to follow the instructions mentioned there.

In my experience, the wrapper approach mentioned in the Node docs doesn't work all that well compared to having Rollup output both formats. I see you're already using Rollup. Is there any reason you don't have it output CJS and ESM instead of creating a wrapper?

Sample Code or Instructions to Reproduce

npm install --save highlight.js
cat node_modules/highlight.js/es/core.js

Expected behavior

The individual functions like highlight and registerLanguage should be exported in addition to the default export

Additional context

I can't get highlight to work in SvelteKit / Vite without workarounds (metonym/svelte-highlight#158)

@benmccann benmccann added bug help welcome Could use help from community parser labels Jul 31, 2021
@joshgoebel
Copy link
Member

joshgoebel commented Jul 31, 2021

it does not appear to follow the instructions mentioned there.

How so specifically?

The individual functions like highlight and registerLanguage should be exported

This is not required (AFAICT) and the instructions go on to state that explicitly:

If the module is not simply a list of named exports, but rather contains a unique function or object export like module.exports = function () { ... }, or if support in the wrapper for the import pkg from 'pkg' pattern is desired, then the wrapper would instead be written to export the default...

HLJS is designed to provide a singleton instance, so we've always exported an instance (as default)... it makes sense to continue to export this main instance in ESM just as it did in CJS.

Is there any reason you don't have it output CJS and ESM instead of creating a wrapper?

It's right in the comments, though I guess the link was subtly off: https://nodejs.org/api/packages.html#packages_dual_package_hazard

This forces a SINGLE instance of HLJS, even if both ESM and CJS are libs imported, this is why a wrapper.

@joshgoebel
Copy link
Member

joshgoebel commented Jul 31, 2021

I can't get highlight to work in SvelteKit / Vite

I don't know what this means exactly... does your project fail to build? What is the error? Does it simply fail at runtime, is there a console error, etc?

It's worth noting the CDN build now also includes ESM libs, I wonder if that might perhaps be of use to you. It is done differently there since we don't have to take Node idiosyncrasies into account.

@benmccann
Copy link
Author

The error I'm getting is:

500
The requested module '/node_modules/highlight.js/lib/core.js?v=2e5442a7' does not provide an export named 'default'
SyntaxError: The requested module '/node_modules/highlight.js/lib/core.js?v=2e5442a7' does not provide an export named 'default'

The problem is that SvelteKit runs as an isomorphic web app. It expects the same code to be able to be used on both the client and server. That's not possible when you have to use default exports because default exports need to be referenced differently depending on whether the package is CJS or ESM.

It would also be good practice to define the module entry point in package.json pointing at the ESM build and an exports map in package.json. I've found that those things help substantially in consuming libraries with Vite. The issue here is that you can't simply point inside the package at a specific CJS or ESM build but need to point at the package entry point so that Vite can load the CJS version on the server and the ESM version on web and have the same piece of code work on both the client and server without any changes

@joshgoebel
Copy link
Member

joshgoebel commented Aug 15, 2021

That's not possible when you have to use default exports because default exports need to be referenced differently depending on whether the package is CJS or ESM.

So use us as ESM everywhere? Not sure I follow the problem.

It would also be good practice to define the module entry point in package.json pointing at the ESM build and an exports map in package.json.

We already have an exports map. We purposely chose to stay CJS for main for version 11. Adding module to point to the ESM build may be doable if it doesn't break anything... do you know? Would that somehow magically solve your export problem?

The issue here is that you can't simply point inside the package at a specific CJS or ESM build

Sadly, to me, that sounds like a limitation your bundling software perhaps needs to address. (if it's causing real pain with integration)


We've provided only a default export for years with no issues at all... when we started publishing our ESM builds and people starting using them also, no issues (with our default export). I'm not opposed to ALSO adding a hljs named export, but I'm not sure why it's necessary and I also think that might break some other more popular packaging systems that currently work just fine. I think? in the past there have been various issues with having BOTH a default export and named exports.

@joshgoebel
Copy link
Member

joshgoebel commented Aug 15, 2021

I'm not opposed to ALSO adding a default named export

If we did do this we'd still only have two exports:

  • default: HLJS singleton
  • "hljs" named: HLJS singleton

We wouldn't be exporting individual functions.

@joshgoebel
Copy link
Member

Are you using our highlight.js package or @highlightjs/cdn-assets?

@joshgoebel
Copy link
Member

Are you talking about something like this? #3307

@benmccann
Copy link
Author

Yes, #3307 should work if it also added a named export to the CJS version as well.

I'm using the highlight.js package

@joshgoebel
Copy link
Member

joshgoebel commented Aug 15, 2021

if it also added a named export to the CJS version as well.

How is that done?

module.exports = hljs;
module.exports.hljs = hljs;

?

@benmccann
Copy link
Author

Yeah. I think that'd work. Or:

hljs.hljs = hljs;
module.exports = hljs;

@joshgoebel
Copy link
Member

@benmccann Could you build and test PR #3307 and see if it works for you?

@joshgoebel
Copy link
Member

@benmccann Ping.

@benmccann
Copy link
Author

Yes, it does work. Thank you!

@joshgoebel
Copy link
Member

I'm curious though now that we're adding default is there a reason you couldn't just import default rather than hljs and thus avoid the need for us to add both?

@benmccann
Copy link
Author

We've actually gotten svelte-highlight working with SvelteKit without any highlight.js changes now. I think perhaps there was some fix in Vite or vite-plugin-svelte to enable it

@joshgoebel
Copy link
Member

So technically from your point of view this is no longer necessary? If there is a chance default will work I'd prefer just to have the one rather than the both.

@benmccann
Copy link
Author

Yeah, we shouldn't need this anymore even if it'd be good practice to use named exports instead of default exports

joshgoebel added a commit that referenced this issue Oct 17, 2021
Resolves #3333. Resolves #3295.

- exports a `HighlightJS` named export
- exports a `default` named export for CJS (TS compatibility)
@joshgoebel
Copy link
Member

Published with 11.3. The export is now (also) named as HighlightJS.

@benmccann
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help welcome Could use help from community parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants