feat(skipLast): add skipLast operator #2316

martinsik · 2017-01-30T13:48:17Z

Add the operator to Rx
It must have a -spec.ts tests file covering the canonical corner cases, with marble diagram tests
If possible, write a asDiagram test case too, for PNG diagram generation purposes
The operator must be documented in JSDoc style in the implementation file, including also the PNG marble diagram image
The operator should be listed in doc/operators.md in a category of operators
It should also be inserted in the operator decision tree file doc/decision-tree-widget/tree.yml
You may need to update MIGRATION.md if the operator differs from the corresponding one in RxJS v4

Description:

Adds skipLast operator from RxJS 4. Its internals and tests are based on takeLast for better performance.

Related issue (if exists):

Adds skipLast operator from RxJS 4. Its internals and tests are based on takeLast for better performance. Closes ReactiveX#1404

martinsik · 2017-01-30T13:50:01Z

I'm actually not sure what the last unchecked bullet point means:

The spec file should have a type definition test at the end of the spec to verify type definition for various use cases

Is there any other operator that already tests this?

coveralls · 2017-01-30T13:52:32Z

Coverage increased (+0.01%) to 97.713% when pulling 0940b14 on martinsik:skiplast-operator into d4533c4 on ReactiveX:master.

staltz · 2017-01-30T14:31:13Z

You can ignore that task, it used to be needed.

staltz · 2017-01-30T14:32:36Z

doc/decision-tree-widget/tree.yml

+        children:
+        - label: based on a given amount
+          children:
+          - label: skipLast


Because there is only one child under "from the end of the Observable", I suggest you combine them into one. So:

- label: from the end of the Observable children: - label: skipLast

staltz · 2017-01-30T14:34:25Z

spec/operators/skipLast-spec.ts

+  asDiagram('skipLast(2)')('should skip two values of an observable with many values', () => {
+    const e1 =  cold('--a-----b----c---d--|');
+    const e1subs =   '^                   !';
+    const expected = '-------------a---b--|';


There is something really wrong about this marble diagram. The bottom a should not be emitted at the same time as the top c, because when c occurs, we don't actually yet know that it's the second-last emission. We can only know that when the e1 completes. So the expected should be (ab|) when the source completes.

As soon as c is emitted, it is known that a should not be skipped because it is not one of the last 2 elements and can be emitted.

Hmm, funny. I may have misunderstood skipLast. Is this how RxJS 4 works?

It appears to be: https://github.com/Reactive-Extensions/RxJS/blob/master/tests/observable/skiplast.js#L248 .

Yeah, this looks correct. if the skipLast(n) buffer grows to n + 1, we know we can drain to n and emit.

coveralls · 2017-02-01T14:43:23Z

Coverage decreased (-0.003%) to 97.699% when pulling 34eab36 on martinsik:skiplast-operator into d4533c4 on ReactiveX:master.

benlesh · 2017-02-02T23:51:32Z

src/operator/skipLast.ts

+ */
+class SkipLastSubscriber<T> extends Subscriber<T> {
+  private ring: T[] = [];
+  private count: number = 0;


A few things:

rename these to start with underscores. I know we're not doing that everywhere, but we should be with private and protected properties as a signal to non-TS users.

Rename total to _skipCount or _numberToSkip, something more descriptive.

We should be initializing the ring buffer in the ctor to be the same size as the _skipCount, it should perform better than constantly using push.

Instead of keeping a _count, we can keep a _currentIndex or _index, such that when we increment it we do: this._currentIndex = (_currentIndex + 1) % _skipCount; That way it doesn't inflate past where it needs to be.

@Blesh I think we have to keep _count instead of just _currentIndex because we need to know when to start remitting buffered items since the buffer is now fixed size.

benlesh · 2017-02-02T23:52:27Z

src/operator/skipLast.ts

+      const idx = this.count++ % this.total;
+      const oldValue = this.ring[idx];
+
+      this.ring[idx] = value;


be sure if your accessing the same property more than once to pull it out into a const. That way we don't have the cost of looking it up over and over.

benlesh · 2017-02-02T23:55:44Z

Overall this looks good. I don't think skipLast is trivial to implement using existing operators, so this is a solid candidate to re-add to the library. I've just requested a few changes, WRT to performance and naming conventions. (Please do check the perf before and after the changes).

benlesh · 2017-02-02T23:57:44Z

Also, it seems the files added have 100% coverage, so I'm not worried about coveralls complaining.

martinsik · 2017-02-04T11:22:14Z

After the changes performance is roughly the same:

Before:

                                         |                     RxJS 4.1.0 |                     RxJS 5.0.3 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                    skiplast - immediate |                 3,154 (±1.23%) |                40,651 (±1.04%) |          12.89x |        1,189.0%
                                skiplast |                 1,442 (±2.35%) |                18,099 (±1.55%) |          12.55x |        1,155.2%

After:

                                         |                     RxJS 4.1.0 |                     RxJS 5.0.3 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                    skiplast - immediate |                 3,218 (±2.27%) |                42,526 (±0.80%) |          13.21x |        1,221.4%
                                skiplast |                 1,497 (±2.31%) |                19,353 (±1.75%) |          12.92x |        1,192.5%

coveralls · 2017-02-04T11:25:48Z

Coverage decreased (-0.002%) to 97.7% when pulling 83ed187 on martinsik:skiplast-operator into d4533c4 on ReactiveX:master.

benlesh · 2017-02-16T02:20:57Z

@martinsik ... this is great! I'll merge this, but it'll have to wait for the next minor. I think I'd like to get the bug fixes out first in a patch. So it might be a few weeks.

benlesh · 2017-05-09T20:34:53Z

Thank you @martinsik!

lock · 2018-06-06T09:41:43Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

martinsik added 2 commits January 30, 2017 13:57

feat(skipLast): adds skipLast operator

47f5752

Adds skipLast operator from RxJS 4. Its internals and tests are based on takeLast for better performance. Closes ReactiveX#1404

docs(skipLast): updated decision tree, MIGRATION.md and operators.md

0940b14

staltz reviewed Jan 30, 2017

View reviewed changes

docs(skipLast): fix tree.yml as suggested by @staltz

34eab36

benlesh requested changes Feb 2, 2017

View reviewed changes

refactor(skipLast): refactored skipLast operator

83ed187

benlesh added the Next Minor Version label Feb 16, 2017

benlesh approved these changes Feb 16, 2017

View reviewed changes

benlesh merged commit 4ffbbe5 into ReactiveX:master May 9, 2017

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(skipLast): add skipLast operator #2316

feat(skipLast): add skipLast operator #2316

martinsik commented Jan 30, 2017 •

edited

Loading

martinsik commented Jan 30, 2017

coveralls commented Jan 30, 2017

staltz commented Jan 30, 2017

staltz Jan 30, 2017

staltz Jan 30, 2017

mbonneau Jan 30, 2017

staltz Jan 30, 2017

mbonneau Jan 30, 2017

trxcllnt Jan 31, 2017

coveralls commented Feb 1, 2017

benlesh Feb 2, 2017

martinsik Feb 4, 2017 •

edited

Loading

benlesh Feb 2, 2017

benlesh commented Feb 2, 2017

benlesh commented Feb 2, 2017

martinsik commented Feb 4, 2017 •

edited

Loading

coveralls commented Feb 4, 2017

benlesh commented Feb 16, 2017

benlesh commented May 9, 2017

lock bot commented Jun 6, 2018

feat(skipLast): add skipLast operator #2316

feat(skipLast): add skipLast operator #2316

Conversation

martinsik commented Jan 30, 2017 • edited Loading

martinsik commented Jan 30, 2017

coveralls commented Jan 30, 2017

staltz commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 1, 2017

Choose a reason for hiding this comment

martinsik Feb 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benlesh commented Feb 2, 2017

benlesh commented Feb 2, 2017

martinsik commented Feb 4, 2017 • edited Loading

coveralls commented Feb 4, 2017

benlesh commented Feb 16, 2017

benlesh commented May 9, 2017

lock bot commented Jun 6, 2018

martinsik commented Jan 30, 2017 •

edited

Loading

martinsik Feb 4, 2017 •

edited

Loading

martinsik commented Feb 4, 2017 •

edited

Loading