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

fix global undefined on service worker #493

Closed
wants to merge 1 commit into from
Closed

fix global undefined on service worker #493

wants to merge 1 commit into from

Conversation

smeng9
Copy link

@smeng9 smeng9 commented Nov 7, 2022

Fixes #487

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Nov 7, 2022

Do you think it would be possible to add a unit test under https://github.com/nodejs/readable-stream/tree/main/test/browser?

@smeng9
Copy link
Author

smeng9 commented Nov 7, 2022

What kind of tests are we expected to run for this case? Do you mean add a runner-worker.mjs for service worker?

@mcollina
Copy link
Member

mcollina commented Nov 7, 2022

It should run a service worker and get back some data.

@smeng9
Copy link
Author

smeng9 commented Nov 8, 2022

Hi @mcollina the worker setup seems is way more complicated than I thought.

Also this pull request should only target v3.6.0 https://github.com/nodejs/readable-stream/blob/bed7ffa274f5b9e6d0d5c22369e6fe825ded03d2/lib/_stream_readable.js where v4 seems does not have such issue. I cannot find a branch for v3.

Unfortunately, some of our dependencies still uses v3.

@mcollina
Copy link
Member

Can you target the v3.x line with your PR then? The build scripts were redone between them.

@smeng9
Copy link
Author

smeng9 commented Nov 18, 2022

Hi @mcollina I cannot find the 3.x branch here https://github.com/nodejs/readable-stream/branches/stale?page=1 to target my commit with. Can you help to create a 3.x branch in this repo? Thanks!

Then I can fork it and create a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot use readable-stream on service worker
2 participants