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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(deps): remove stream-events dependency #2022

Merged
merged 4 commits into from Aug 9, 2022

Conversation

ddelgrosso1
Copy link
Contributor

@ddelgrosso1 ddelgrosso1 commented Aug 8, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1916 馃

@ddelgrosso1 ddelgrosso1 requested review from a team as code owners August 8, 2022 20:32
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: storage Issues related to the googleapis/nodejs-storage API. labels Aug 8, 2022
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 8, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 8, 2022
Copy link
Contributor

@shaffeeullah shaffeeullah left a comment

Choose a reason for hiding this comment

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

Looks straightforward enough! @danielbankhead pls take a look as well though

@ddelgrosso1 ddelgrosso1 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
@ddelgrosso1 ddelgrosso1 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 8, 2022
Copy link
Member

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

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

Looks good so far

src/util.ts Outdated Show resolved Hide resolved
src/util.ts Show resolved Hide resolved
Comment on lines +172 to +173
private shouldEmitReading = true;
private shouldEmitWriting = true;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this further, 'reading' and 'writing' are called on every _read and _write event - to be fully backwards-compatible this class should do the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch, I misread the stream-events / stubs code. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I do believe it is only called once: https://github.com/stephenplusplus/stubs/blob/580b4d8b3a34c1193d5ec10e0db0d6ea4b4c0ec5/index.js#L30 it looks like after the number of calls is exhausted we revert to the cached function and stream-events would stop emitting reading / writing.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 9, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Aug 9, 2022
@ddelgrosso1 ddelgrosso1 merged commit b1e477e into googleapis:main Aug 9, 2022
@ddelgrosso1 ddelgrosso1 deleted the remove-stream-events branch August 9, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor(deps): Remove stream-events dependency
4 participants