feat: remove WebSocket dependencies

[penovicp]

Motivation and Resolution

The most widely used browsers and the most recent Node.js LTS version provide native WebSocket support. This means that for most users the fallback modules should be unnecessary.

Usage related changes

Most users should be unaffected. Users working with environments without native WebSocket support can use the websocket parameter of the WebSocketChannel constructor to provide a valid WebSocket connection themselves.

Checklist:

• Performed a self-review of the code
• Rebased to the last commit of the target branch (or merged it into my branch)
• Linked the issues which this PR resolves
• Documented the changes in code (API docs will be generated automatically)
• Updated the tests
• All tests are passing

[aryzing]

Suggested change

	(typeof window !== 'undefined' && window.fetch) ||
	(typeof global !== 'undefined' && global.fetch) ||
	(typeof globalThis.fetch !== 'undefined' && globalThis.fetch) ||

Otherwise a discrete "global object" check per environment needs to be added. globalThis is the more modern cross-environment option.

[tabaktoni]

Until everyone becomes modern, we will leave inexpensive tests and also add globalThis

[ondrej-secretkeylabs]

Has this been tested in browsers?

I get Failed to execute 'fetch' on 'Window': Illegal invocation

[aryzing]

Seems it's due to the missing .bind(window) as in:

starknet.js/src/utils/fetch.ts

Line 5 in 960981c

export default (IS_BROWSER && window.fetch.bind(window)) || // use built-in fetch in browser if available

For @penovicp : do you plan on supporting non-fetch-supporting environments? Can we just get rid of these checks that add complexity? All modern environments support native fetch.

[tabaktoni]

I don't see why this is mandatory but will add window.fetch.bind(window)

[aryzing]

https://stackoverflow.com/a/69876888

[victorkirov]

@tabaktoni it's also needed on globalThis and global for the same reason 🙏

  (typeof globalThis !== 'undefined' && globalThis.fetch.bind(globalThis)) ||
  (typeof window !== 'undefined' && window.fetch.bind(window)) ||
  (typeof global !== 'undefined' && global.fetch.bind(global)) ||

and this first line should not exist as it will override all the others as fetch === globalThis.fetch === window.fetch === global.fetch, so this line would always win over the others and the binding won't happen:

(typeof fetch !== 'undefined' && fetch) ||

[aryzing]

Suggested change

	export default (typeof fetch !== 'undefined' && fetch) ||
	(typeof globalThis !== 'undefined' && globalThis.fetch) ||
	export default (typeof globalThis !== 'undefined' && globalThis.fetch.bind(globalThis)) ||

The first check is practically equivalent to the second. The second form is more desirable since it wouldn't make sense to bind to globalThis without first checking for it.

What works and what doesn't:

Works:

const myFetch = fetch;
await myFetch("https://example.com"); // ok

Does not work: Uncaught (in promise) TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation

const myObj = {
  myFetch,
  fetch,
};

myObj.myFetch("https://example.com"); // error
myObj.fetch("https://example.com"); // error

Why? the this value in this second case is myObj.

Why does this fail with starknet? The key areas of the library's compiled code look like this:

// src/utils/connect/fetch.ts
var fetch_default =
  (typeof fetch !== "undefined" && fetch) ||
  (typeof globalThis !== "undefined" && globalThis.fetch) ||
  (typeof window !== "undefined" && window.fetch.bind(window)) ||
  (typeof global !== "undefined" && global.fetch) ||
  (() => {
    throw new LibraryError(
      "'fetch()' not detected, use the 'baseFetch' constructor parameter to set it"
    );
  });

and

this.baseFetch = baseFetch || config.get("fetch") || fetch_default;

with a random invocation looking like,

this.baseFetch(this.nodeUrl, {
  method: "POST",
  body: stringify2(rpcRequestBody),
  headers: this.headers,
});

Above, this is RpcChannel, not window or globalThis, so the call fails.

[tabaktoni]

Ok, but why are you suggesting removing support for node.js fetch?

[victorkirov]

@tabaktoni plz see this comment on why the global fetch without a context should be removed. As per the comment, the bind also needs to be applied to global and globalThis or they will break in the same way that window broke.

[aryzing]

@tabaktoni, the changes suggested in the comment above don't remove support for Node.js fetch. If applied, as also suggested by @victorkirov above, the final result would look like

  (typeof globalThis !== 'undefined' && globalThis.fetch.bind(globalThis)) ||
  (typeof window !== 'undefined' && window.fetch.bind(window)) ||
  (typeof global !== 'undefined' && global.fetch.bind(global)) ||

• globalThis --> all modern clients
• window --> legacy browsers
• global --> legacy node.js

[github-actions]

🎉 This PR is included in version 7.2.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀