wooorm / refractor

Lightweight, robust, elegant virtual syntax highlighting using Prism
MIT License
724 stars 34 forks source link

Incorrect Prism global in browser environment #52

Closed thompsongl closed 3 years ago

thompsongl commented 3 years ago

Related to #38, but in this case focused on browser environments. The global Prism instance is not correctly setting manual or disableWorkerMessageHandler as intended in core.js: https://github.com/wooorm/refractor/blob/e399dffa4017f8f2a639568fd92a408e76e7c23d/lib/core.js#L59

Screen Shot 2021-09-21 at 10 06 04 AM

The result is that code elements are automatically processed by Prism, and in my case altering the DOM in an unwanted manner.

Screen Shot 2021-09-21 at 10 05 19 AM

Minimal reproduction in codesandbox: https://codesandbox.io/s/stoic-mccarthy-9fjzr?file=/src/App.js

N.B.: I think this only affects v4.x; window.Prism is not defined in v3.4.0

wooorm commented 3 years ago
thompsongl commented 3 years ago

Thanks for the quick reply, @wooorm!

Something as simple as

// Inherit.
function Refractor() {}

+ Prism.manual = true;
+ Prism.disableWorkerMessageHandler = true;

Refractor.prototype = Prism

in core.js#L76 appears to do the trick, but I'm lacking background on the reason for the ctx logic.

wooorm commented 3 years ago

Hmm. The Prism code is a bit complex to read. They suggest setting window.Prism = {manual: true} to not auto-highlight the document.

With your technique, I’m guessing that this line still runs, but this one doesn’t? 🤔

thompsongl commented 3 years ago

This might be better:

- ctx.Prism = {manual: true, disableWorkerMessageHandler: true}
+ ctx.Prism = ctx.Prism || {};
+ ctx.Prism.manual = true;
+ ctx.Prism.disableWorkerMessageHandler = true;

Appears to resolve the problem.

wooorm commented 3 years ago

/cc @rexxars, as you today forked refractor, and opened GH-38 — thoughts on this?

thompsongl commented 3 years ago

👍 I'm happy to open a PR if we all agree this is the right approach

thompsongl commented 3 years ago

Created #53 after confirming it fixes the browser environment problem. Happy make changes if more is needed to also resolve #38

rexxars commented 3 years ago

I never managed to fully isolate and reproduce #38, but the suggested fix seems logical to me