-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Comments
Apologies for the incorrect labels |
I'm not sure I know enough to comment here. We are simply doing plain old boring 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 |
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 So to fix this, you have a couple of options:
As a package maintainer, an issue with 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 |
esModuleInterop
to true.
The more specific typescript flag is |
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?
|
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. |
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 What if you just import the es module manually?
|
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. |
I wouldn't know how to tell Typescript to do that, other than your workaround below --v
This emits:
Fair enough. I love this project, and I'm not complaining. I'm just reporting that when I updated to v11 this broke =) |
Can we just declare an additional ambient module for that in |
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:
I know some people want named exports, but we've done |
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
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 =) |
Would it help if I prepare a |
That'd be the magic to know... :)
Can't hurt, and
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. |
TypeScript could also get their act together and respect the hints already inside |
Hey guys - I just stumbled accross this issue as I'm having the same problem. 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 Switching from The issue at hand is, that the ESM- and the CJS-version of A solution would be to change the CJS-version to As this would be a breaking change, there's also an alternative workaround: highlight.js/tools/build_node.js Line 22 in bd548da
to this: const footer = "hljs.default = hljs;\nmodule.exports = hljs;"; In doing so, both the ESM- and the CJS-version of Furthermore, However - this is only my personal suggestion. |
@evert A workaround for you until this issue gets solved: Add the following to your "paths": {
"highlight.js": [
"./node_modules/@types/highlight.js"
]
} Make sure you have the most recent version of |
I always thought these were equivalencies - perhaps that's only because how Rollup complies things...
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 Thanks for your effort 😄 |
Ping. @manuth |
I just gave it a go right now! Thanks for your effort there 😄 |
Published with 11.3 |
Confirmed to work for me! Thank you!! |
Describe the issue/behavior that seems buggy
To use highlight.js without typescript complaining, I need to do:
However, this gives me an error that hljs is
undefined
.To fix this, I need to do this:
I think this is because typescript expects you to export an object with a propertyname
default
.The text was updated successfully, but these errors were encountered: