-
Notifications
You must be signed in to change notification settings - Fork 797
feat: remove WebSocket dependencies #1374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,11 @@ | ||||||||
import { LibraryError } from '../errors'; | ||||||||
|
||||||||
export default (typeof fetch !== 'undefined' && fetch) || | ||||||||
(typeof globalThis !== 'undefined' && globalThis.fetch) || | ||||||||
Comment on lines
+3
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The first check is practically equivalent to the second. The second form is more desirable since it wouldn't make sense to bind to What works and what doesn't: Works: const myFetch = fetch;
await myFetch("https://example.com"); // ok Does not work: const myObj = {
myFetch,
fetch,
};
myObj.myFetch("https://example.com"); // error
myObj.fetch("https://example.com"); // error Why? the Why does this fail with // 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, but why are you suggesting removing support for node.js fetch? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tabaktoni plz see this comment on why the global There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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)) ||
|
||||||||
(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" | ||||||||
); | ||||||||
}) as WindowOrWorkerGlobalScope['fetch']); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { LibraryError } from '../errors'; | ||
|
||
export default (typeof WebSocket !== 'undefined' && WebSocket) || | ||
(typeof globalThis !== 'undefined' && globalThis.WebSocket) || | ||
(typeof window !== 'undefined' && window.WebSocket.bind(window)) || | ||
(typeof global !== 'undefined' && global.WebSocket) || | ||
(class { | ||
constructor() { | ||
throw new LibraryError( | ||
"WebSocket module not detected, use the 'websocket' constructor parameter to set a compatible connection" | ||
); | ||
} | ||
} as unknown as typeof WebSocket); |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been tested in browsers?
I get
Failed to execute 'fetch' on 'Window': Illegal invocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's due to the missing
.bind(window)
as in:starknet.js/src/utils/fetch.ts
Line 5 in 960981c
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this is mandatory but will add window.fetch.bind(window)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://stackoverflow.com/a/69876888
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tabaktoni it's also needed on globalThis and global for the same reason 🙏
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: