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

feat(timeout): One timeout to rule them all #5602

Merged
merged 2 commits into from
Aug 2, 2020

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jul 23, 2020

  • Adds configuration argument pattern
  • Fixes issue where timeout and timeoutWith would always timeout when provided a Date as a due time, even if the source emitted a value before then.
  • Adds ability to configure timeout to have a different first timeout check and subsequent each timeout checks
  • Adds ability to timeout if just the first value does not arrive after a specified number of milliseconds
  • Adds additional information to TimeoutErrors thrown by bare timeout calls.
  • Adds with parameter to timeout operator's configuration, such that now timeout can be used in exactly the same ways as timeoutWith and more. The with parameter is a function that provides information about the timeout and expects an ObservableInput to switch to in response to a timeout.
  • Deprecates timeoutWith.
  • Updates tests to use run mode.
  • Adds additional tests to timeout to cover more aspects of the configuration possiblities.
  • Improves type signatures for timeout and timeoutWith.

Basic New Usage Patterns

IN ADDITION to the existing functionality of timeout, the timeout operator now can accept a single configuration object that allows better control over timeouts.

source$.pipe(
   timeout({ 
      // An absolute date by which the first value must arrive
      first: new Date('10/22/2021'),
      // The length of time between values allowed (after the date provided by first)
      each: 1000,
      // optional information to help identify the source of the timeout
      meta: { reason: 'Too slow!' },
      with: (info) => {
          // If a timeout occurs, switch to this observable
          return of(`Timeout out after ${info.seen} values. Reason: ${info.meta.reason}. Last value: ${info.lastValue}`);
      }
   })
)
source$.pipe(
   // Since the new `with` property was not specified, will error with `TimeoutError`, as usual
   timeout({ 
      // If the first value does not arrive within 4 seconds, timeout.
      // After that, values may arrive after any delay, (each was not specified)
      first: 4000,
      // optional information to help identify the source of the timeout
      meta: { reason: 'Too slow!' },
   }),
   catchError(err => {
      if (err instanceof TimeoutError) {
          // Timeout error provides more information, including how many values were seen, the last value seen, and the metadata specified in the timeout config.
          return of(`Timeout out after ${err.info.seen} values. Reason: ${error.info.meta.reason}. Last value: ${error.info.lastValue}`);
      }
      throw err;
   });
)

TODO:

  • Update docs
  • Add a few more tests

@benlesh
Copy link
Member Author

benlesh commented Jul 23, 2020

Should resolve the concerns here: #4537

@benlesh
Copy link
Member Author

benlesh commented Jul 23, 2020

I'm game to change the name of with. I was just aiming for understandability from timeoutWith... however, that's deprecated by this PR, and honestly, I'm not sure it's used as much as timeout.

So the with property could be called something like switchTo, or using or the like. Whatever makes the most sense to people.

@benlesh benlesh requested review from kwonoj, cartant, kolodny and niklas-wortmann and removed request for kwonoj July 23, 2020 06:48
@kolodny
Copy link
Member

kolodny commented Jul 24, 2020

A couple of API points. This seems like a kitchen sink function (not a bad thing), in which case we may want to allow more flexibility. What do you think about replacing first and each with delayAllowed or timeAllowed and have it
take a number | Date | ((info: Info) => number | Date). That would also allow the more flexibility in subsequent items and can fine tune the time based on the current time, previous values, as well something like () => min(moment().add('4 seconds'), APP_DEADLINE).

Also the meta and with seem redundant and can be merged into a timeoutWith (not 100% on that name either TBH). Also I'm wondering if it makes sense for the timeoutWith to just become timeoutReason and be a string that's always wrapped in a TimeoutError, 95% of the time the user won't switch this to a plain obs value but if they didn't want it to actually throw they could just pipe directly on it

Taking your examples above it would turn into:

source$.pipe(
  timeout({
    // First item must happen by '10/22/2021, after that every 1 second
    timeAllowed: info => info.seen === 0 ? new Date('10/22/2021') : 1000
    timeoutReason: info => `Timeout out after ${info.seen} values. Reason: 'Too slow!'. Last value: ${info.lastValue}`,
  }
})
)


source$.pipe(
  timeout({ 
    // If the first value does not arrive within 4 seconds, timeout.
    // After that, values may arrive after any delay
    timeAllowed: info => info.seen === 0 ? 4000 : Infinity
  }).pipe(
    catchError(err =>
      of(`Timeout out after ${err.info.seen} values. Reason:''Too slow!''. Last value: ${error.info.lastValue}`);
    )
  ),
)

I know I kind of have the examples backwards where the first example throws and the second doesn't but I think the point still gets across.

Thoughts?

@benlesh
Copy link
Member Author

benlesh commented Jul 25, 2020

@kolodny I've wondered if the meta is redundant. Because you can just do:

timeout({
  first: 5000,
  with: () => { throw new CustomError('meta stuff here'); }
})

However this means that CustomError will have to either be it's own error class, extending Error, OR, if people still want to catch it "downstream" with an instanceof TimeoutError, they'll need to subclass TimeoutError, which I'm not a huge fan of. I'm trying to think of what the best course of action is here, but I'm not quite sure.

As far as the names go, I was trying to be minimalist, but maybe they missed the mark, I'm not sure. I do know I'm not a fan of the word timeout appearing twice in the API.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

A bunch of nits and some opinions. Still thinking about the meta business.

api_guard/dist/types/index.d.ts Show resolved Hide resolved
spec/operators/timeout-spec.ts Outdated Show resolved Hide resolved
spec/operators/timeout-spec.ts Outdated Show resolved Hide resolved
spec/operators/timeout-spec.ts Outdated Show resolved Hide resolved
spec/operators/timeout-spec.ts Outdated Show resolved Hide resolved
src/internal/operators/timeoutWith.ts Outdated Show resolved Hide resolved
src/internal/operators/timeout.ts Outdated Show resolved Hide resolved
src/internal/operators/timeout.ts Outdated Show resolved Hide resolved
src/internal/operators/timeout.ts Outdated Show resolved Hide resolved
src/internal/operators/timeout.ts Outdated Show resolved Hide resolved
@benlesh benlesh requested a review from cartant August 2, 2020 01:10
@benlesh benlesh marked this pull request as ready for review August 2, 2020 01:11
dependabot bot and others added 2 commits August 2, 2020 13:05
…eactiveX#5594)

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.15 to 4.17.19.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.15...4.17.19)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- Adds configuration argument pattern
- Fixes issue where `timeout` and `timeoutWith` would always timeout when provided a Date as a due time, even if the source emitted a value before then.
- Adds ability to configure timeout to have a different `first` timeout check and subsequent `each` timeout checks
- Adds ability to timeout if _just_ the first value does not arrive after a specified number of milliseconds
- Adds additional information to `TimeoutError`s thrown by bare `timeout` calls.
- Adds `with` parameter to `timeout` operator's configuration, such that now `timeout` can be used in exactly the same ways as `timeoutWith` and more. The `with` parameter is a function that provides information about the timeout and expects an `ObservableInput` to switch to in response to a timeout.
- Deprecates `timeoutWith`.
- Updates tests to use run mode.
- Adds additional tests to `timeout` to cover more aspects of the configuration possiblities.
- Improves type signatures for `timeout` and `timeoutWith`.
@benlesh benlesh merged commit def1d34 into ReactiveX:master Aug 2, 2020
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.

3 participants