Util require is not needed

[jorgecasar]

TextEncoder and TextDecoder are native supported from Nodejs 10 and Nodejs 8.0 is not supported since 2020. It make sense to set Nodejs 10 as minimum supported version (engines in package.json). And then we can remove util require.

It also make this library available for browser environment that actually supports wasm too.

[jorgecasar]

@srenatus it works, could you merge and release a new version. Some colleagues need this to make it works in the browser. thanks!

[srenatus]

@jorgecasar would you have a minute to look into what's happening here? From your message I'd have expected the other PR #108 to not cause any trouble, but it seems like it did - tests failed after rebasing the other PR onto main.

[srenatus]

FWIW I've been using this in the browser before, see open-policy-agent/community#35. Hmm looks like I was using 1.2.0, so TextEncoder/Decoder wasn't used back then...? 🤔

I'll have another look into this next week if nobody beats me to it. 🤞

The other PR had enabled >=10.0.0 in support node engines, so anything we merge after now should be properly checked for node 10/12/14.

[aron]

Took a quick look at this as you messaged me in #108, TextEncoder has been available in Node since 8.3.0 but only behind the util module. The global constructor was added in Node 11 which is why you're seeing the test suite failing.

I would suggest the simplest path forward would be to use a conditional to check for the global instance and otherwise do a dynamic require: e.g.

const TextEncoder = typeof global.TextEncoder !== "undefined" ? global.TextEncoder : require('util').TextEncoder;

This will work on Node 10+ but require whatever bundler is being used to convert the commonjs modules into something consumable by a browser (e.g. webpack or rollup) to correctly alias global (which I think is always the case).

[jorgecasar]

Oh, then we can support 12 and next. Node 10 is not supported since last year. We can set >= 12 in the engines. What do you think?

https://nodejs.org/en/about/releases/

[aron]

I was just poking around to see if this would work and put together a very quick demo. As far as I can tell this patch will work on Node 10+ as well as the browser. I tested in Firefox, Node 10 and Node 16.

https://github.com/aron/demo-opa-wasm-browser-build

[srenatus]

Oh, then we can support 12 and next. Node 10 is not supported since last year. We can set >= 12 in the engines. What do you think?

Technically yeah, but since @aron already provided a fix, and they're still depending on Node 10 (as mentioned in #108), I'd tend to go down that route.

@aron would you mind opening a PR with https://gist.github.com/aron/41eac08e8f1469a1c8139766524d3bad? I also think your demo repo could be very useful if it was part of this one, like, examples/browser, or something.

[aron]

@srenatus I gave this a little more thought, and I think from the perspective of this project and making it easier for web consumption it might make sense to provide a browser compatible esm build as part of the npm bundle and include a pointer via the browser flag in the package.json. I see you have #38 which also touches on this, later you could also provide commonjs and esm node versions too.

In the mean time I can look into opening a PR for the patch for you though.

[srenatus]

@jorgecasar what do you think? Do you have the bundle setup on your end to make this work for you (#107 (comment))?

[srenatus]

In the mean time I can look into opening a PR for the patch for you though.

Thinking about it, to use this in a browser you would need to use some sort of bundler with the patch and without it, right? And the proper fix would be the browser-compatible ESM bundle. So, I guess I'm torn.... waiting for what @jorgecasar has to say.

[jorgecasar]

I was working on it. I have the source Conde in ESM and using Typescript I generate the Commons. I will create a draft PR to test the ci because in my local environment running nom test doesn't work