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

Highlight.js requires Typescript users to set esModuleInterop to true. #3333

Closed
evert opened this issue Sep 14, 2021 · 24 comments · Fixed by #3307
Closed

Highlight.js requires Typescript users to set esModuleInterop to true. #3333

evert opened this issue Sep 14, 2021 · 24 comments · Fixed by #3307
Labels
bug help welcome Could use help from community parser
Milestone

Comments

@evert
Copy link

evert commented Sep 14, 2021

Describe the issue/behavior that seems buggy

To use highlight.js without typescript complaining, I need to do:

import hljs from 'highlight.js`

hljs.highlight(/* etc */)

However, this gives me an error that hljs is undefined.

To fix this, I need to do this:

import * as hljs from 'highlight.js`

// @ts-ignore-error 
hljs.highlight(/* etc */)

I think this is because typescript expects you to export an object with a propertyname default.

@evert evert added bug help welcome Could use help from community parser labels Sep 14, 2021
@evert
Copy link
Author

evert commented Sep 14, 2021

Apologies for the incorrect labels

@joshgoebel
Copy link
Member

joshgoebel commented Sep 15, 2021

I'm not sure I know enough to comment here. We are simply doing plain old boring export default right now (and have for some time) and it works for most everyone. The first example you show is how you import us, as shown in our docs:

https://github.com/highlightjs/highlight.js#es6-modules--import

We have tests in our CI suite that do this exact thing... so I'm not sure why TS is unhappy. This "property named default" is new AFAIK and should not be a requirement - as we've never done it before and this is the first I'm hearing of issues.

Which build package of Highlight.js are you using exactly?

Are you saying there is something wrong with our index.d.ts file or you simply don't think we are doing exports properly in our JS distributable?

@evert
Copy link
Author

evert commented Sep 15, 2021

Hi @joshgoebel ,

This is using the latest v11.2.0 npm package, and this actually broke since upgrading from version 10.

I did some more digging, and noticed that the error goes away if my package has esModuleInterop set to true in its tsconfig.json. After turning that on, the package works as documented.

So to fix this, you have a couple of options:

  1. Document that esModuleInterop is required to use this package with typescript. I would probably also add to VERSION_11_UPGRADE.md as (at least to me) was a bc break.
  2. Remove the need for this flag

As a package maintainer, an issue with esModuleInterop is that you kind of require everyone that uses your library to also have this flag on. It's a bit infectious in that way. I'm using this package in a library, so to use highlight.js without errors, both my package and my users all need to set this flag to true.

Because of this, I decided some time ago that all my libraries shouldn't rely on this flag, because if you don't, you basically leave it up to the end user to decide if they want to use the flag or not. If it's set to true, you take away this choice.

@evert evert changed the title Typescript types seem wrong Highlight.js requires Typescript users to set esModuleInterop to true. Sep 15, 2021
@evert
Copy link
Author

evert commented Sep 15, 2021

The more specific typescript flag is allowSyntheticDefaultImports, which automatically turns on when esModuleInterop is on.

@joshgoebel
Copy link
Member

This is using the latest v11.2.0 npm package

CJS or ESM? From what I read this flag is all about CJS modules... if you want ESM, why not use the bundled ESM modules instead?

By default (with esModuleInterop false or not set) TypeScript treats CommonJS/AMD/UMD modules similar to ES6 modules.

@evert
Copy link
Author

evert commented Sep 15, 2021

Most people (myself included) will not emit ESM with my typescript build. Most people will basically emit CJS modules when using typescript.

I'm not sure if that if I set typescript to a ESM target, it will automatically use ESM modules from dependencies, but my setup is pretty common and it will use highlight.js's CJS out of the box.

@joshgoebel
Copy link
Member

I'm not talking about what you're emitting, I'm talking about what you're consuming. The suggestion would be don't consume our CJS modules if Typescript doesn't like them, consume our ESM modules (which we've shipped since v11).

AFAIK we specify all the proper "magic" in package.json for this to happen automagically, but perhaps TypeScript doesn't respect this?

What if you just import the es module manually?

import hljs from 'highlight.js/es/index.js`

@joshgoebel
Copy link
Member

joshgoebel commented Sep 15, 2021

and this actually broke since upgrading from version 10.

The entire build system changed, but AFAIK we're still exporting a single export, as we always have. Tons of other stuff changed though like shipping dual CJS/EMS, etc.

@evert
Copy link
Author

evert commented Sep 15, 2021

I'm not talking about what you're emitting, I'm talking about what you're consuming. The suggestion would be don't consume our CJS modules if Typescript doesn't like them, consume our ESM modules (which we've shipped since v11).

I wouldn't know how to tell Typescript to do that, other than your workaround below --v

What if you just import the es module manually?

import hljs from 'highlight.js/es/index.js`

This emits:

Could not find a declaration file for module 'highlight.js/es/index.js'.

The entire build system changed, but AFAIK we're still exporting a single export, as we always have. Tons of other stuff changed though like shipping dual CJS/EMS, etc.

Fair enough. I love this project, and I'm not complaining. I'm just reporting that when I updated to v11 this broke =)

@joshgoebel
Copy link
Member

Could not find a declaration file for module 'highlight.js/es/index.js'.

Can we just declare an additional ambient module for that in index.d.ts like we do for languages already?

@joshgoebel
Copy link
Member

Fair enough. I love this project, and I'm not complaining. I'm just reporting that when I updated to v11 this broke =)

Oh sure, which is great. I'm just slow to react on these TS issue because I find usually the first thing people suggest is often wrong and that the problems are typically much more complex and nuanced.

So right now I'd like to first know/understand:

  • How do we make our es modules work with TS (which seems relevant since this whole interop thing seems to be an CJS issue only from how I read it)
  • Why our default export is broken with TS for our CJS builds which simply do module.exports = highlight;

I know some people want named exports, but we've done module.exports for as long as I can remember without issue and I thought this was a super common pattern.

@evert
Copy link
Author

evert commented Sep 15, 2021

Can we just declare an additional ambient module for that in index.d.ts like we do for languages already?

Yes, you could, and it would solve my immediate problem.

I think this is solvable in the top-level import too though, I just don't know exactly what incantations are needed. I mostly write pure typescript and less familiar with definition files.

Because I think if I were maintaining highlight.js, I would prefer that everyone can use the same method for importing, regardless of their settings.

Otherwise you might end up with documentation like:

// Normally you'd import like this in typescript:

import hljs from 'highlight.js';

// But if `esModuleInterop` is set to false, you may need:

import hljs from 'highlight.js/es/index.js';

If we can remove the second option, I think everyone will be happier

Oh sure, which is great. I'm just slow to react on these TS issue because I find usually the first thing people suggest is often wrong and that the problems are typically much more complex and nuanced.

You've been fast and I super appreciate you're taking the time =) I hope my tone wasn't demanding or entitled! JS modules and how they interact with tooling is tricky =)

@evert
Copy link
Author

evert commented Sep 15, 2021

Would it help if I prepare a tsconfig.json and test.ts file that demonstrates the issue?

@joshgoebel
Copy link
Member

I think this is solvable in the top-level import too though,

That'd be the magic to know... :)

Would it help if I prepare a tsconfig.json and test.ts file that demonstrates the issue?

Can't hurt, and package.json for installing all the needs deps, etc... a tiny self-contained repo would be perfect.

If we can remove the second option, I think everyone will be happier

Well if I'd listened to some we wouldn't have shipped CJS anymore at all and people would have just dealt with it, so this whole "two ways" is only a short-term problem at best. I do think an all-ESM world is coming soon enough.

@joshgoebel
Copy link
Member

TypeScript could also get their act together and respect the hints already inside package.json telling it to use the ES modules automagically, but perhaps they have their reasons... :-)

@manuth
Copy link

manuth commented Sep 18, 2021

Hey guys - I just stumbled accross this issue as I'm having the same problem.
First things first: TypeScript indeed does respect the hints found inside package.json-files.

However: You can't consume ESM-code from a CJS-project - thtat's not an issue on TypeScript's side but something that simply doesn't work. If you try to do so, you'd get an error saying Cannot use import statement outside a module. Thus, if you import modules from a CJS module, the CJS-version will be imported (using require() as hinted in the package.json) and if you import modules from an ESM module, the ESM-version will be imported using an import-statement as hinted in the package.json.

Switching from ESM to CJS isn't an option for many people as there are many cases where you just can't. I, for my part, am writing vscode extensions and yeoman generators and both of them don't support extensions and/or generators written in ESM yet. So something like writing import hljs from 'highlight.js/es/index.js'; won't help.

The issue at hand is, that the ESM- and the CJS-version of highlight.js don't look the same.
One of them uses export default hljs (a default export) while the other one uses module.exports = hljs (an export-assignment).

A solution would be to change the CJS-version to module.exports.default = hljs to make it look the same like the ESM-version.

As this would be a breaking change, there's also an alternative workaround:
I'd suggest to change this piece of code:

const footer = "module.exports = hljs;";

to this:

 const footer = "hljs.default = hljs;\nmodule.exports = hljs;";

In doing so, both the ESM- and the CJS-version of highlight.js would look the same (as both provide a default export), thus the type-declarations would work out no matter whether you emit a CJS or an ESM-module and no matter whether esModuleInterop is enabled.

Furthermore, const hljs = require("hljs"); would still work (as both the import-assignment and the default export are set to hljs).

However - this is only my personal suggestion.

@manuth
Copy link

manuth commented Sep 18, 2021

@evert A workaround for you until this issue gets solved:
Install a working version of @types/highlight.js in your module using npm install @types/highlight.js@9.12.4.

Add the following to your tsconfig.jsons compilerOptions:

        "paths": {
            "highlight.js": [
                "./node_modules/@types/highlight.js"
            ]
        }

Make sure you have the most recent version of typescript installed in your project - otherwise you have to set baseUrl in your compilerOptions, too, which might have unwanted side-effects (incorrect suggestions when autocompleting import-statements).
Also - take note that in doing so, you're bound to use deprecated API as pointed out in #2277
However - it works around the issue currently.

@joshgoebel
Copy link
Member

One of them uses export default hljs (a default export) while the other one uses module.exports = hljs (an export-assignment).

I always thought these were equivalencies - perhaps that's only because how Rollup complies things...

As this would be a breaking change, there's also an alternative workaround:

This seems reasonable... but I feel like I've seen issues where build systems (Rollup) will then complain that they don't like seeing this "mixed" type of export (default vs named exports)... but perhaps I'm thinking of something else - or that wouldn't apply here.

I might see if we can slip this into #3307 and see how it works.

@manuth
Copy link

manuth commented Sep 18, 2021

One of them uses export default hljs (a default export) while the other one uses module.exports = hljs (an export-assignment).

I always thought these were equivalencies - perhaps that's only because how Rollup complies things...

As this would be a breaking change, there's also an alternative workaround:

This seems reasonable... but I feel like I've seen issues where build systems (Rollup) will then complain that they don't like seeing this "mixed" type of export (default vs named exports)... but perhaps I'm thinking of something else - or that wouldn't apply here.

I might see if we can slip this into #3307 and see how it works.

Yeah - as far as I can tell, even microbuild compiles export default x to module.exports = x. However - TypeScript treats export default and module.exports = differently (both in CJS ESM).

Thanks for your effort 😄

@joshgoebel
Copy link
Member

@manuth Can you build from source (PR #3307) and drop in the package and see if TS is happy now?

@joshgoebel
Copy link
Member

Ping. @manuth

@manuth
Copy link

manuth commented Oct 6, 2021

I just gave it a go right now!
It works like a charm

Thanks for your effort there 😄

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

@evert
Copy link
Author

evert commented Oct 18, 2021

Confirmed to work for me! Thank you!!

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.

3 participants