digitalbazaar / cborld

A Javascript CBOR-LD processor for web browsers and Node.js apps.
https://json-ld.github.io/cbor-ld-spec/
BSD 3-Clause "New" or "Revised" License
21 stars 17 forks source link

fix: add matchAll support for older versions of node #18

Closed tplooker closed 3 years ago

tplooker commented 3 years ago

String.matchAll() support was only added to node.js after version 12.0.0, see here. This PR adds support for using the npm package match-all when running the library in such an environment.

davidlehn commented 3 years ago
tplooker commented 3 years ago

v10 is a few months from end-of-life. Any chance you'd rather just update? I had to ask.

Most of our stuff is based on v14, except for one library we use in RN thats a dep in one of our mono repos, so forces us to run certain tests back on v10 and consequently this library was causing issues. Happy to explore another path if you're not looking to continue support for Node 10? Just figured you were based on the GH actions configured node environment including v10.

The test suite does test v10, so I imagine that code path isn't tested. Do you know of a test that could be added for this?

Yes just after I submitted the PR I began wondering why prior tests hadn't failed, i'll see if I can add something.

I don't know how to force github actions to run for those who are not part of the repo organization. One thing to note is I'm guessing the linter would have some issues

Will run locally and amend any issues, had not checked to see that CI hadn't kicked off.

This isn't the best construct for this type of support.

Ok can look at abstracting this a bit more to prevent loading the un-needed deps in browser, was largely just following the pattern I had seen in one of your libraries here

tplooker commented 3 years ago

@davidlehn this should be resolved now?

tplooker commented 3 years ago

Ping @msporny @davidlehn is there anything further I can do with this PR to get it merged?

tplooker commented 3 years ago

Closing in favour of PR #19