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.

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

Drop detect-node dependency by coalescing eventsource inclusion #831

Merged
merged 4 commits into from
Jun 13, 2023

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented May 22, 2023

What

Simplifies usage of eventsource in the different environments:

  • First, using ?? is much cleaner than the nonsense if (...defined...) cases
  • Second, there's no need for isNode detection: we should just try to require it unconditionally if we can't find it somewhere in the globals or the window object

Historical context


Why

Related: #824, since it drops the detect-node dependency.

Bundle size

Assets: 
  stellar-sdk.js (1.88 MiB)
  stellar-sdk.min.js (684 KiB)

@Shaptic Shaptic changed the title Coalesce eventsource inclusion, drop Node detection Drop detect-node dependency by coalescing eventsource inclusion May 22, 2023
// 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.

Copy link

@aristidesstaffieri aristidesstaffieri left a comment

Choose a reason for hiding this comment

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

awesome yeah I think the feature detection approach is better than env detection

@Shaptic Shaptic merged commit 52947e8 into master Jun 13, 2023
@Shaptic Shaptic deleted the coalesce-eventsource branch June 13, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants