Skip to content

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