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(core): Disconnect from all adapters when pause is called #291

Merged
merged 5 commits into from
Jan 12, 2020

Conversation

offirgolan
Copy link
Collaborator

Description

Calling polly.pause() will now disconnect from all connected adapters instead of setting the mode to passthrough. Calling polly.play() will reconnect to the disconnected adapters before pause was called.

Motivation and Context

Resolves #285.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • My code follows the code style of this project.
  • My commits and the title of this PR follow the Conventional Commits Specification.
  • I have read the contributing guidelines.

BREAKING CHANGE: Calling `polly.pause()` will now disconnect from all connected adapters instead of setting the mode to passthrough. Calling `polly.play()` will reconnect to the disconnected adapters before pause was called.
@offirgolan offirgolan added bug 🐞 Something isn't working 📦 core labels Jan 10, 2020
@offirgolan offirgolan requested a review from jasonmit January 10, 2020 18:58
@@ -256,23 +271,35 @@ describe('Unit | Polly', function() {
});

it('.pause()', async function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add a test asserting on the behavior of _order when a pause and play occurs with requests in between. Will prevent a regression but also solidify expected behavior.

@jasonmit
Copy link
Contributor

Side note: should note this is a breaking change in the commit msg

@offirgolan offirgolan requested a review from jasonmit January 11, 2020 19:48
.travis.yml Outdated
@@ -17,7 +17,7 @@ before_install:
- export PATH=$HOME/.yarn/bin:$PATH

install:
- yarn install --no-lockfile --non-interactive
- yarn install --frozen-lockfile --non-interactive
Copy link
Contributor

@jasonmit jasonmit Jan 12, 2020

Choose a reason for hiding this comment

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

I think we want to keep this to ensure polly still works with all the latest deps. Otherwise, we won't pick up on breaks until they're reported.

It makes CI pipeline more fragile but it's important to see those breaks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. It looks like node semver v7 is no longer supporting node 8 which is breaking our node 8 build.

Node 8 is EOL as of this year, should we drop support as well? I suspect more and more packages will be doing the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, agree to drop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, I'll address in a follow up PR as this is unrelated.

@offirgolan offirgolan merged commit 5c655bf into master Jan 12, 2020
@offirgolan offirgolan deleted the disconnect-adapters-on-pause branch January 12, 2020 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working 📦 core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should "_order" still increment after polly.pause?
2 participants