Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Drop detect-node dependency by coalescing eventsource inclusion #831

Merged
merged 4 commits into from
Jun 13, 2023
Merged

Drop detect-node dependency by coalescing eventsource inclusion #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b66698
Coalesce eventsource inclusion, drop Node detection
Shaptic May 22, 2023
021ebca
Fixup the `prettier` ignore paths and run it"
Shaptic May 22, 2023
b88e083
Moar question marks :fist_oncoming:
Shaptic May 22, 2023
bc553b0
Refresh yarn.lock after new deps
Shaptic May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions config/.prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/package.json
/node_modules
/lib
/dist
/docs
gulpfile.js
../package.json
../node_modules
../lib
../dist
../docs
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,6 @@
"dependencies": {
"axios": "^1.4.0",
"bignumber.js": "^9.1.1",
"detect-node": "^2.0.4",
"eventsource": "^2.0.2",
"randombytes": "^2.1.0",
"stellar-base": "v9.0.0-beta.2",
Expand Down
22 changes: 5 additions & 17 deletions src/call_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import isNode from "detect-node";
import URI from "urijs";
import URITemplate from "urijs/src/URITemplate";

Expand All @@ -14,29 +13,18 @@ const version = require("../package.json").version;
// query-param.
const JOINABLE = ["transaction"];

type Constructable<T> = new (e: string) => T;

export interface EventSourceOptions<T> {
onmessage?: (value: T) => void;
onerror?: (event: MessageEvent) => void;
reconnectTimeout?: number;
}

let EventSource: Constructable<EventSource>;
const anyGlobal = global as any;

if (anyGlobal.EventSource) {
EventSource = anyGlobal.EventSource;
} else if (isNode) {
/* tslint:disable-next-line:no-var-requires */
EventSource = require("eventsource");
} else if (anyGlobal.window.EventSource) {
EventSource = anyGlobal.window.EventSource;
} else {
// require("eventsource") for React Native environment
/* tslint:disable-next-line:no-var-requires */
EventSource = require("eventsource");
}
type Constructable<T> = new (e: string) => T;
// require("eventsource") for Node and React Native environment
let EventSource: Constructable<EventSource> = anyGlobal.EventSource ??
anyGlobal.window?.EventSource ??
require("eventsource");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old isNode package was checking to see if process is defined instead of checking if window is defined (which it looks like this solution is doing).

I've run into an issue in Freighter where I've tried to run stellar sdk in a service worker and because window wasn't defined, it falsely assumed I was in a node environment, which has led to some issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the PR that addressed the mentioned Freighter issue: stellar/js-stellar-base#567

It might not actually matter here, but thought I would mention

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good context, thanks! You're right that we're doing a window check, but I think it should still be okay here because the default logic flow stays the same. Can you double-check my rationale?

Previously:

  1. is there an EventSource in the global variable? use it
  2. is there a process (via isNode)? then require() it
  3. is there a window in the global variable AND an EventSource on that window? use it
  4. default to trying to require() it

Now, we drop (2) and move it to the "default case." I think this is fine, because if we're in Node, there won't be a window, and if there is a window, we can still use its EventSource if it has one. Then if all else fails, we try importing it.


/**
* Creates a new {@link CallBuilder} pointed to server defined by serverUrl.
Expand Down
Loading