feat: make Buffer optional

[manzt]

Fixes #12

Thanks for this library! We use it extensively in https://github.com/higlass/higlass.

We recently upgraded our build and ran into #12. As mentioned in that issue, uuid does not have a dependency on Buffer,
so we should be able to refactor the slugid internals and not require browser users to have a large Buffer polyfill. I made a shim for slugid in HiGlass (higlass/higlass#1112), but I'd really prefer to keep a compatible slugid as a dependency.

Changes

• adds toBase64 and fromBase64 which are built on Buffer or atob/btoa
• test suite runs twice, once with Buffer as a global and once with Buffer removed from globalThis.

btoa and atob are deprecated in Node.js, so this PR inspects globalThis for a Buffer. If it's present (polyfill or Node),
then it will use the module. Otherwise it is assumed the environment has atob and btoa (browser), and will avoid the use of Buffer for
encoding/decoding uuids.

cc: @pkerpedjiev

[djmitche]

Thanks for the PR! But, it looks like it's failing on older node versions. Hopefully that shouldn't be too hard to patch up?

[djmitche]

Also, it may make more sense to evaluate the condition(s) once, rather than on every call? What do you think?

[manzt]

Thanks for the PR! But, it looks like it's failing on older node versions. Hopefully that shouldn't be too hard to patch up?

This issue is that globalThis is a cross environment "global" which wasn't added until after Node.js v12. In the browser, it is an alias for window or self and in Node is it global. (There is no global in the browser). Node.js <= 12 has been EOL for more than 4 months, so I think it would be worth considering the more modern syntax. The other option would be to add an helper to find the global.

EDIT: I just pushed some changes which I think will work, but some bundlers will try to add Buffer if they see it like this even if it is not required.

Also, it may make more sense to evaluate the condition(s) once, rather than on every call? What do you think?

The conditions are only evaluated once on import. toBase64 and fromBase64 are IIFEs which inspect the global for Buffer and return the appropriate function (without a conditional).

[djmitche]

Hah, sorry on both counts -- I've not been doing JS for a while!

[manzt]

No problem! I think the current changes should work with well with bundlers.

[manzt]

Herm... I think btoa/atob were added and then deprecated in Node.js... which is now causing the new errors. The issue is that we are trying to run all the tests with and without Buffer in all versions of Node.js. In Node, we should always get the implementation where Buffer is used since it is globally available across all versions.

The second branch (without Buffer) is only meant to run in environments which don't have Buffer, like a browser. We can emulate this in Modern node versions (since they have btoa/atob) so we probably just want to test NO_BUFFER=1 nodeunit slugid_test.js in those environments and not later.

If we were allowed the use of globalThis (and removed tests for Node.js <=12), I believe we could test this easier.

[djmitche]

Yeah, that's fair -- go ahead and drop Node <= 12 support (in GitHub actions and, lol, package.json says node >= 6!!).

Also, I just realized I no longer have permission to merge things on this repo, so we'll need to wait for one of the Taskcluster devs to apply their own review and merge.

[manzt]

I guess what I am saying is that this repo is technically compatible with node >= 6 (if we don't use globalThis), so I don't think we need to deprecate anything. That said, since all those versions are well past EOL it might be worth it to drop support and save some CPUs. I think we just have the test runner do:

• npm test in v6/v8/v10/v12/v14/v16/latest
• NO_BUFFER=1 npm test in v16/latest

I'd be happy to write that up as a GH Action workflow, but I am not very familiar with .taskcluser.yml

[djmitche]

Ah, I can make that change.

[djmitche]

Haha, well, I can't actually push that change, but something like

diff --git a/.taskcluster.yml b/.taskcluster.yml
index 1c80e40..788a7c3 100644
--- a/.taskcluster.yml
+++ b/.taskcluster.yml
@@ -9,28 +9,38 @@ tasks:
       else: ${event.after}
     repository:
       $if: tasks_for == "github-pull-request"
       then: ${event.pull_request.head.repo.html_url}
       else: ${event.repository.html_url}
     environments:
       - image: node:6
         description: Run tests with node v6
+        command: npm test
       - image: node:8
         description: Run tests with node v8
+        command: npm test
       - image: node:10
         description: Run tests with node v10
+        command: npm test
       - image: node:12
         description: Run tests with node v12
+        command: npm test
       - image: node:14
         description: Run tests with node v14
+        command: npm test
       - image: node:16
         description: Run tests with node v16
+        command: npm test
       - image: node:latest
         description: Run tests with latest node
+        command: npm test
+      - image: node:latest
+        description: Run tests with latest node without Buffer
+        command: npm test-nobuffer
   in:
     $if: tasks_for == "github-pull-request" && event["action"] in ["opened","reopened","synchronize"]
     then:
       $map: {$eval: environments}
       each(env):
         taskId: {$eval: as_slugid(env.image + ":tests")}
         deadline:
           $fromNow: 1 day
@@ -52,9 +62,9 @@ tasks:
               git --version &&
               node -v &&
               npm -v &&
               git clone --no-progress ${repository} repo &&
               cd repo &&
               git config advice.detachedHead false &&
               git checkout --no-progress ${head_rev} &&
               npm install --no-progress . &&
-              npm test
+              ${env.command}

with "test-nobuffer" added to package.json should do the trick. And, I think we only need to test the no-buffer case with the latest nodejs, since it's not really testing a nodejs-based scenario. (really, we should be testing in a browser, but this is good enough)

[edit: added env.command on the last line]

[manzt]

Ok, thanks!

[community-tc-integration]

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[3]: object has no property "command"

[manzt]

Think I made a mistake in the config, sorry!

(really, we should be testing in a browser, but this is good enough)

Yes agreed, but hopefully this is an ok proxy for the time being.

[community-tc-integration]

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[3]: object has no property "command"

[community-tc-integration]

Uh oh! Looks like an error! Details

InterpreterError at template.tasks.payload.command[3]: object has no property "command"

[manzt]

Thanks!

[lotas]

Thanks @manzt for the contribution and @djmitche for the review :)
I will merge it now

[lotas]

I've just published v3.1.0 with those changes. Thanks once again