wooorm / lowlight

Virtual syntax highlighting for virtual DOMs and non-HTML things
MIT License
746 stars 26 forks source link

Running highlight.js after using lowlight results in blank output from highlight.js #47

Closed segevfiner closed 1 year ago

segevfiner commented 2 years ago

This is caused by the following: https://github.com/wooorm/lowlight/blob/ebddb1040b8b4adc78abb502e59ddd378a56bd8b/lib/core.js#L61-L67

configure changes highlight.js globals, but calling it with an empty object does not restore those globals. Leading to highlight.js being left in a broken state.

I'm not sure how to properly workaround this ATM. e.g. To force a separate copy of highlight.js for the use of lowlight.

Reproduction: https://stackblitz.com/edit/vitejs-vite-mjpizz?file=src/components/HelloWorld.vue

wooorm commented 2 years ago

@joshgoebel is there any way to get the original configuration from highlight again? Or, is TokenTreeEmitter exposed (I don’t think it is?)?

joshgoebel commented 2 years ago

It's not. I have two thoughts here off the top of my head.

I think allowing outside code to reach into the internals isn't optimal - so the idea of lowlight potentially preserving and then restoring the state isn't really the right approach IMHO.

Could lowlight potentially use an isolated instance and leave the global alone? Any other thoughts/ideas?

wooorm commented 2 years ago

Previously, const previousConfig = high.configure({}) was called. And then later high.configure(previousConfig) to revert. I can’t see code in highlight.js to suggest that that ever worked. But it gives another idea for a fix: return the effective configuration from configure. That would solve things for me.

then restoring the state isn't really the right approach IMHO.

Could lowlight potentially use an isolated instance and leave the global alone? Any other thoughts/ideas?

How? HLJS is made as a global instance. I’m hacking around that. If there’s a viable alternative I’d want to use it. But I don’t think there ever was.

joshgoebel commented 2 years ago

return the effective configuration from configure

Allowing outside code to potentially destroy/break the object is weird and incompatible ways. I suppose we could return a copy...

How? HLJS is made as a global instance. I’m hacking around that.

We'd add a new API: HLJS.newInstance() (or some such) that gave you a whole new copy... and I could probably be talked into making the ESM version (with v12) just return the class, not an instance... so everyone in ESM world would need to do new HighlightJS(), and there would be no more "default" HLJS object... thoughts?

wooorm commented 2 years ago

potentially destroy/break the object

Yeah, sure. On the other hand, people can already pass weird values and break things anyway: https://github.com/highlightjs/highlight.js/blob/bc88ed5c03c9d9f1255bd420a08784c5e9db62b4/src/highlight.js#L778.

that gave you a whole new copy

Yes please!

just return the class, not an instance

YES PLEASE! Sounds great. Main thing for me is that I’d rather have something soon. So the first option (HLJS.newInstance) is enough. But, I think that (new HighlightJS) would be better in the end!

joshgoebel commented 1 year ago

I'd take a PR.. we probably need something like:

  return hljs;
};

const main_instance = HLJS({});
main_instance.newInstance = () => HLJS({})

// export an "instance" of the highlighter
export default main_instance;

And then make sure that doesn't break any of the build scripts (which sometimes do specific things with the build output at the tail end, etc).

wisammechano commented 1 year ago

I am running into this issue currently, and considering either stop using react-syntax-highlighter, or saving the configuration somewhere.

I am using Quill with ReactMarkdown, Quill needs a highlightAuto from hljs to work, however, the emitter changes from TokenTreeEmitter to a generic Emitter which still looks like HastEmitter based on its rootNode children type.

wooorm commented 1 year ago

Please see the comment before your comment: a PR is welcome. You can fix it.

dschwank commented 1 year ago

Just stumbled over this issue. As you mentioned @joshgoebel , best would be to make a fix in the highlight.js library, to either get the config or get a clean instance. As you are the current maintainer of that project, do you see a chance to get that into highlight.js v11 / v12? Or is that already on the roadmap? If not, can we support you somehow?

joshgoebel commented 1 year ago

This happened some time back: https://github.com/highlightjs/highlight.js/commit/1f084d1f0b3eac791af7e1846d5b4bf0754b7bef

I had thought lowlight was already taking advantage...

dschwank commented 1 year ago

@joshgoebel Thanks for those great news. I checked the changelog, but missed that part! Will take a closer look tomorrow and might create a PR!

Thanks a lot!

wooorm commented 1 year ago

Released in 3.0.0!