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 forced eventsource polyfill to opt-in to streaming support #878

Closed
wants to merge 0 commits into from

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Nov 3, 2023

The presence of the eventsource dependency is a problem for many deployment environments. It's also a polyfill that is non-essential for environments with native support for the EventSource API. Furthermore, not everyone needs streaming support.

This PR drops this dependency from being installed by default and warns downstream about the implications if the API is not present.

Its presence is still necessary in devDependencies for running tests related to streaming. Downstream developers can avoid installing these by using npm i --production stellar-sdk.

@Shaptic Shaptic added the dependencies Pull requests that update a dependency file label Nov 3, 2023
@Shaptic Shaptic requested a review from kalepail November 3, 2023 21:56
@Shaptic Shaptic self-assigned this Nov 3, 2023
Copy link

github-actions bot commented Nov 3, 2023

Size Change: -1.56 MB (-14%) 👏

Total Size: 9.64 MB

Filename Size Change
dist/stellar-sdk.js 5.38 MB -807 kB (-13%) 👏
dist/stellar-sdk.min.js 4.26 MB -753 kB (-15%) 👏

compressed-size-action

Copy link
Contributor

@silence48 silence48 left a comment

Choose a reason for hiding this comment

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

Let's merge it. beautiful idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love it and strongly support!

Copy link
Contributor

@kalepail kalepail left a comment

Choose a reason for hiding this comment

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

Lgtm! Haven't tested though. Would that be helpful before merging or are we confident in the change?

@Shaptic
Copy link
Contributor Author

Shaptic commented Nov 3, 2023

@tyvdh it'd be super helpful lol

@kalepail
Copy link
Contributor

kalepail commented Nov 4, 2023

Get a different error. But still an error

✘ [ERROR] Could not resolve "https"

    node_modules/.pnpm/github.com+stellar+js-stellar-sdk@29aaf8a11a17b34de65feeddd60f033125af22a9/node_modules/stellar-sdk/node_modules/eventsource/lib/eventsource.js:3:20:
      3 │ var https = require('https')
        ╵                     ~~~~~~~

  The package "https" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node.js compatibility.


✘ [ERROR] Could not resolve "http"

    node_modules/.pnpm/github.com+stellar+js-stellar-sdk@29aaf8a11a17b34de65feeddd60f033125af22a9/node_modules/stellar-sdk/node_modules/eventsource/lib/eventsource.js:4:19:
      4 │ var http = require('http')
        ╵                    ~~~~~~

  The package "http" wasn't found on the file system but is built into node.
  Add "node_compat = true" to your wrangler.toml file to enable Node.js compatibility.

@kalepail
Copy link
Contributor

kalepail commented Nov 4, 2023

I'm using pnpm vs npm so not sure if or how they handle this --production flag. I'm not familiar with that flag. If I use npm directly with that flag I get this error

service core:user:colorglyph-worker: Uncaught Error: Cannot find module 'eventsource'
  at index.js:53676:28 in webpackMissingModule
  at index.js:53679:18 in 6881
  at index.js:75582:43 in __webpack_require__
  at index.js:57323:61 in 8026
  at index.js:75582:43 in __webpack_require__
  at index.js:49844:59 in 5085
  at index.js:75582:43 in __webpack_require__
  at index.js:75656:37
  at index.js:75658:11
  at index.js:36374:26 in webpackUniversalModuleDefinition

@kalepail
Copy link
Contributor

kalepail commented Nov 4, 2023

@Shaptic here's a boilerplate repo for testing on a CF worker
https://github.com/tyvdh/stellar-worker-boilerplate
Hopefully proves helpful :)

Copy link

socket-security bot commented Nov 15, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@haverstack/axios-fetch-adapter 0.8.0 network +1 413 kB cuibonobo

@Shaptic
Copy link
Contributor Author

Shaptic commented Nov 15, 2023

@tyvdh give it another shot! I think I got it 😎

image

@Shaptic Shaptic force-pushed the make-eventsource-optional branch 3 times, most recently from 89a6f39 to 65442da Compare November 15, 2023 22:10
@kalepail
Copy link
Contributor

Just confirming I have tested this and it's working fine. SHIP IT
Not a huge fan of unmuteable alarms/alerts but that's a nit

@Shaptic
Copy link
Contributor Author

Shaptic commented Nov 28, 2023

@tyvdh lol the warning works if webpack is missing, but a subsequent npm i eventsource does not remove/fix the warning 😅

@kalepail
Copy link
Contributor

Would love to see this branch updated and merged. I use it a lot and it's starting to fall behind

@kalepail
Copy link
Contributor

Well 💩
I added a remote and accidentally pushed here instead of to my remote
Then I tried to revert
And now somehow I've closed this PR and can't seem to reopen it.
Oof

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants