[browser] window access not guarded by typeof check

[peterjuras]

Hi! :wave:

Firstly, thanks for your work on this project! :slightly_smiling_face:

Today I used patch-package to patch ably@1.2.36 for the project I'm working on.

I want to use ably inside the forge runtime by Atlassian to add realtime functionality for Jira dashboard gadgets. Their runtime is not a full Node.js environment and therefore the node version of the package is not working. The browser version appears to be working for my purposes (createTokenRequests), therefore I want to use it.

The browser version - which is required by the forge CLI by default - has an unguarded access to the window object, which throws an error in Node.js context. Could you please consider adding the guard to it - or even better - provide a Node.js runtime which does run on a slimmed down version?

Here is the diff that solved my problem:

diff --git a/node_modules/ably/build/ably-commonjs.js b/node_modules/ably/build/ably-commonjs.js
index a44c7b2..d6f3031 100644
--- a/node_modules/ably/build/ably-commonjs.js
+++ b/node_modules/ably/build/ably-commonjs.js
@@ -15,7 +15,7 @@
        exports["Ably"] = factory();
    else
        root["Ably"] = factory();
-})(window, function() {
+})(typeof window !== 'undefined' && window, function() {
 return /******/ (function(modules) { // webpackBootstrap
 /******/   // The module cache
 /******/   var installedModules = {};

This issue body was partially generated by patch-package.

┆Issue is synchronized with this Jira Task by Unito

[owenpearson]

Hey @peterjuras, thanks for raising this! The access to the window object here is autogenerated by webpack so wouldn't be trivial to remove, and ably-commonjs.js is a web distribution so even if we avoid window access there would be no guarantee of stability on non-web platforms for future versions. I think patching the Node.js distribution is a more viable option, would you be able to tell us what error you're seeing when you use the node package in forge? If your use case is just to create signed token requests and nothing else it may also be worth considering doing it without an ably SDK, see these docs for a guide on how to do that.

[sync-by-unito[bot]]

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3391

[peterjuras]

Hi @owenpearson ,

thanks for the reply! I now completed my first use case with ably within the forge runtime, and I had to patch a couple of more things.

I saw that it is possible to generate signed token requests without the ably SDK, but I'd like to avoid that to not have to maintain the code.

For the Atlassian forge runtime, I had to work around the following issues, since it uses the browser version of packages by default:

• Patch the unsafe window usage as mentioned in my initial post
• Patch the usage of fetch within the code to use import { fetch } from "@forge/api" instead of the global fetch. This also included replacing some usages of a global Header function (e.g. new Header(...).

It would be nice, if you could enable the ably JS SDK to work out of the box with the Atlassian forge runtime. Realistically however, I understand that this might be a niche use case.

Given the trend of multiple vendors creating their own slimmed down JavaScript runtimes (e.g. Cloudflare Workers, Lambda@Edge, Vercel Edge functions, Azure's version, etc.) this creates an interesting problem for libraries where there is no longer a single version of "Node.js", but multiple flavours with different supported APIs.

[owenpearson]

Patch the unsafe window usage as mentioned in my initial post

As mentioned before, I'm not totally convinced that guarding access to window is a good idea for a distribution which is targeted towards web browsers. You mentioned earlier that the node version of the package is not working in forge but it would be helpful to know exactly what's failing there.

Incidentally, we do have a webworker bundle which is similar to the browser version but doesn't use the window object. You can import it with import * as Ably from 'ably/build/ably-webworker.min (we can also provide a non-minified version if there's an interest in it).

Patch the usage of fetch within the code to use import { fetch } from "@forge/api" instead of the global fetch. This also included replacing some usages of a global Header function (e.g. new Header(...).

I don't see us adding explicit support for something like this but it could be an option to add support for a user-provided http request implementation, ie a function passed in as a client option which takes method, body, qsparams, etc and actually makes the request. I would have to have a think about how this would work w.r.t retries/timeouts etc but it likely wouldn't be too complicated.

Given the trend of multiple vendors creating their own slimmed down JavaScript runtimes (e.g. Cloudflare Workers, Lambda@Edge, Vercel Edge functions, Azure's version, etc.) this creates an interesting problem for libraries where there is no longer a single version of "Node.js", but multiple flavours with different supported APIs.

Well said 🙂