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

offirgolan commented Jan 10, 2020

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.
@@ -256,23 +271,35 @@ describe('Unit | Polly', function() {
});

it('.pause()', async function() {

This comment has been minimized.

Copy link
@jasonmit

jasonmit Jan 11, 2020

Collaborator

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

This comment has been minimized.

Copy link
Collaborator

jasonmit commented Jan 11, 2020

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

@offirgolan offirgolan requested a review from jasonmit Jan 11, 2020
.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

This comment has been minimized.

Copy link
@jasonmit

jasonmit Jan 12, 2020

Collaborator

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.

This comment has been minimized.

Copy link
@offirgolan

offirgolan Jan 12, 2020

Author Collaborator

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.

This comment has been minimized.

Copy link
@jasonmit

jasonmit Jan 12, 2020

Collaborator

Yup, agree to drop

This comment has been minimized.

Copy link
@offirgolan

offirgolan Jan 12, 2020

Author Collaborator

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

This reverts commit 35d93f5.
@offirgolan offirgolan merged commit 5c655bf into master Jan 12, 2020
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
@offirgolan offirgolan deleted the disconnect-adapters-on-pause branch Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.