wooorm / starry-night

Syntax highlighting, like GitHub
MIT License
1.45k stars 30 forks source link

Add support for custom URLs to WASM binaries #15

Closed sebastinez closed 1 year ago

sebastinez commented 1 year ago

To allow users to provide a URL to a locally hosted oniguruma WASM binary, this commit allows users to provide a function that returns a URL object to a different WASM oniguruma binary, it should .

The options object can receive a getOnigurumaUrlFetch and a getOnigurumaUrlFs property for browser and node.

Closes #8

sebastinez commented 1 year ago

Thank you for working on this!!!

My pleasure, looking forward to using it on my own.

The ## Types section in the readme should include Options and GetOnigurumaUrl too.

Have listed them inline with the Grammar and Root

These changes need to be tested. I think it’s fine to only test the *Fs version. I think it’s fine to return a bogus URL there and catch the error.

Added to the test.js file, as you explained

Optionally, you can add an example. It can be quite short likely. To show your use case: self-hosting onig.wasm on your own website.

Added in e922348

One gnarly part is still that for this project, updating vscode-oniguruma, is likely unrelated to semver versioning, as it is internal, which means that people that use these features, will be broken at a point in the future: #8 (comment). I’d like to discuss this further. Do you have ideas on how to handle that? Even if we would cut a major on a vscode-oniguruma, and explain it to users, vscode-oniguruma might also change internals in patches/minors, which we pull in, but if people have a static file on a server, there will be people that forget to update, whose websites will break.

Yeah I thought about that, but I'm not sure I have a good answer, I think for now we should probably have a bigger Disclaimer in the readme and maybe in the jsdoc comments, so people know they can shoot themselves in the foot by using it.. Maybe if it shows that more people use it, and start have issues with it we can revert this PR.

wooorm commented 1 year ago

@sebastinez Btw, if you go to the “Files changed” tab, you can click each suggestion and add them to a batch, and then commit them in one go, for the ones you agree with!

sebastinez commented 1 year ago

@sebastinez Btw, if you go to the “Files changed” tab, you can click each suggestion and add them to a batch, and then commit them in one go, for the ones you agree with!

Thanks @wooorm I prefer to handle it locally in my working copy and then push the final version.

sebastinez commented 1 year ago

It seems that the tests run now fine, and the linter doesn't comply. I also added a Disclaimer that talks about eventual breaking if the used static WASM differs from the one pulled in from the CDN

If you are ok with it, please lmk and I will squash the commits down to one, before you merge it

sebastinez commented 1 year ago

Hmm ok nevermind CI says otherwise 😓 will look into them

wooorm commented 1 year ago

Thanks for your terrific work, released in 1.5.0!

sebastinez commented 1 year ago

Thanks @wooorm for your help and support in merging this PR! Really appreciate your work 💪🙏