Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

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

Merged
ddelgrosso1 merged 4 commits into
googleapis:mainfrom
ddelgrosso1:remove-stream-events
Aug 9, 2022
Merged

refactor(deps): remove stream-events dependency#2022
ddelgrosso1 merged 4 commits into
googleapis:mainfrom
ddelgrosso1:remove-stream-events

Conversation

@ddelgrosso1

@ddelgrosso1 ddelgrosso1 commented Aug 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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
shaffeeullah
shaffeeullah approved these changes Aug 8, 2022
View reviewed changes

@shaffeeullah shaffeeullah left a comment

Copy link
Copy Markdown
Contributor

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
danielbankhead
danielbankhead reviewed Aug 8, 2022
View reviewed changes

@danielbankhead danielbankhead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good so far

Comment thread src/util.ts Resolved
Comment thread src/util.ts Resolved
danielbankhead
danielbankhead reviewed Aug 8, 2022
View reviewed changes
Comment thread src/util.ts
Comment on lines +172 to +173
private shouldEmitReading = true;
private shouldEmitWriting = true;

@danielbankhead danielbankhead Aug 8, 2022

Copy link
Copy Markdown
Contributor

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

@ddelgrosso1 ddelgrosso1 Aug 9, 2022

Copy link
Copy Markdown
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.

@ddelgrosso1 ddelgrosso1 Aug 9, 2022

Copy link
Copy Markdown
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.

@danielbankhead danielbankhead mentioned this pull request Aug 9, 2022
Closed
@ddelgrosso1
ddelgrosso1 force-pushed the remove-stream-events branch from 6a62184 to 46494ce Compare August 9, 2022 16:35
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@danielbankhead danielbankhead danielbankhead approved these changes
@shaffeeullah shaffeeullah shaffeeullah approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the googleapis/nodejs-storage API. size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor(deps): Remove stream-events dependency

4 participants

@ddelgrosso1 @danielbankhead @shaffeeullah @yoshi-kokoro