-
Notifications
You must be signed in to change notification settings - Fork 313
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
Conversation
detect-node
dependency by coalescing eventsource
inclusion
// require("eventsource") for Node and React Native environment | ||
let EventSource: Constructable<EventSource> = anyGlobal.EventSource ?? | ||
anyGlobal.window?.EventSource ?? | ||
require("eventsource"); |
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.
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.
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.
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
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.
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:
- is there an
EventSource
in theglobal
variable? use it - is there a
process
(viaisNode
)? thenrequire()
it - is there a
window
in the global variable AND anEventSource
on thatwindow
? use it - 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.
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.
awesome yeah I think the feature detection approach is better than env detection
341a55d
to
6b21617
Compare
6b21617
to
bc553b0
Compare
What
Simplifies usage of
eventsource
in the different environments:??
is much cleaner than the nonsenseif (...defined...)
casesisNode
detection: we should just try torequire
it unconditionally if we can't find it somewhere in theglobal
s or thewindow
objectHistorical context
Why
Related: #824, since it drops the
detect-node
dependency.Bundle size