search

wooorm / refractor

Lightweight, robust, elegant virtual syntax highlighting using Prism
MIT License
722 stars 33 forks source link

Bug: Can we change global to globalThis? #36

Closed xiaoxiangmoe closed 3 years ago

xiaoxiangmoe commented 3 years ago

https://github.com/wooorm/refractor/blob/0e49fc33bfac9ae201a815a1e33b3a53152cf73a/core.js#L221

In browsers we have no global, but we have globalThis both in browsers and nodejs

wooorm commented 3 years ago

Did you run into an error?

xiaoxiangmoe commented 3 years ago

Yes! I'm using @angular/cli which using webpack as bundler. image image

wooorm commented 3 years ago

Hmm. Do you know if angular/cli is using webpack 5 now?

This line has been in there since the beginning, millions of users have used it just fine, so I’m assuming bundlers have always added it, but also know that webpack 5 changed some similar things.

xiaoxiangmoe commented 3 years ago

No, angular/cli use webpack 4. Maybe we should support webpack 5, also.

wooorm commented 3 years ago

In that case, I’m assuming this still works fine in webpack 5 (because that’s been out a bit and nobody else raised an issue for it), but that angular/cli is the root problem.

Regardless, I’d appreciate a PR!

wooorm commented 3 years ago

Oh btw, sorry I forgot: I’m not sure globalThis is a good idea. As it doesn’t work everywhere, and that could be a breaking change.

I think we’re already getting the global scope somewhere else in the code. Perhaps use that?

xiaoxiangmoe commented 3 years ago

https://github.com/wooorm/refractor/blob/0e49fc33bfac9ae201a815a1e33b3a53152cf73a/core.js#L9-L14

Do you mean we should use ctx？

wooorm commented 3 years ago

I think that’d do the trick yeah. But maybe that’d also need to support Node’s global?

xiaoxiangmoe commented 3 years ago

see https://github.com/wooorm/refractor/pull/37

xiaoxiangmoe commented 3 years ago

Another polyfill for globalThis: https://github.com/ungap/global-this/blob/master/index.js