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.

Sign up for GitHub

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

Use native iterators instead of Lumino iterators #346

Merged
merged 62 commits into from
Aug 17, 2022
+765 −2,151
Merged

Use native iterators instead of Lumino iterators #346

Changes from 1 commit
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
8639b66
Refactor some of the public API for iterators
afshin Aug 10, 2022
a935f6a
More iterator refactoring
afshin Aug 10, 2022
029bcc3
Update each<T>() function
afshin Aug 10, 2022
c88f20b
Remove ArrayIterator<T>
afshin Aug 10, 2022
f5176d6
Remove KeyIterator, ValueIterator<T>, ItemIterator<T>, and FnIterator<T>
afshin Aug 10, 2022
150738a
Remove zip<T>() and ZipIterator<T>
afshin Aug 10, 2022
ddeab32
Remove take<T>() and TakeIterator<T>
afshin Aug 10, 2022
ebc44cc
Remove toArray<T>()
afshin Aug 10, 2022
d40cc10
Remove ChainIterator<T> and reimplement chain<T>() with native iterators
afshin Aug 10, 2022
77a44ae
Remove RepeatIterator<T> and reimplement repeat<T>() and once<T>() wi…
afshin Aug 11, 2022
d1b6e2e
Reimplement take<T>() with native iterators
afshin Aug 11, 2022
4c13c6f
Reimplment zip<T>() with native iterators
afshin Aug 11, 2022
588e666
Reimplment stride<T>() with native iterators
afshin Aug 11, 2022
346cd2a
Update topologicSort<T>() to accept native iterable
afshin Aug 11, 2022
de3de59
Reimplment retro<T>() with native iterators
afshin Aug 11, 2022
acaa6ed
Update reduce() to use native iterables
afshin Aug 11, 2022
fb48177
Reimplment range() with native iterators
afshin Aug 11, 2022
e6964f3
Reimplement empty() with native iterators
afshin Aug 11, 2022
462f495
Reimplement filter<T>() with native interators
afshin Aug 11, 2022
6abfc62
Reimplement map<T>() with native interators
afshin Aug 11, 2022
298ac8c
Update find(), findIndex(), min(), max(), and minmax() to use native …
afshin Aug 11, 2022
9380340
Reimplement enumerate() with native types
afshin Aug 11, 2022
ccbb647
Fix algorithm package tests
afshin Aug 11, 2022
518079c
Update LinkedList to use native iterators
afshin Aug 11, 2022
481cfb7
Fix LinkedList tests
afshin Aug 11, 2022
82d9b28
Update disposable
afshin Aug 11, 2022
b4c1250
Update widgets to use native iterators
afshin Aug 11, 2022
3ab8cb1
Fix widget tests
afshin Aug 11, 2022
3d633c4
Update data grid to use native iterators
afshin Aug 11, 2022
4f1a5c1
Update API report
afshin Aug 11, 2022
797ea47
Fix doc strings
afshin Aug 11, 2022
fbc612a
Minor tweak of `LinkedList` iterator methods
afshin Aug 13, 2022
720ce2e
Simplify `chain<T>()`
afshin Aug 13, 2022
35d6f40
Simplify `take<T>()`
afshin Aug 13, 2022
050daeb
[Symbol.iterator]() and Array.from() should almost never need invoked
afshin Aug 13, 2022
d6012a4
Third parameter for comparator functions is superfluous when they are…
afshin Aug 13, 2022
0f2ec49
Simplify stride<T>()
afshin Aug 13, 2022
68eb1dd
Re-enable zip() tests
afshin Aug 13, 2022
a5bc2c3
Simplify iterating through selections in data grid.
afshin Aug 13, 2022
f0189cc
Tweak retro<T>() for readability
afshin Aug 13, 2022
8a9574a
Use for...of for minmax<T>()
afshin Aug 14, 2022
fbf1317
Use `IterableIterator` as return type for all generators
afshin Aug 14, 2022
654aef5
Use for...of instead of each() in disposable and update generated API
afshin Aug 15, 2022
14aa4da
Use for...of instead of each() in LinkedList and topologicSort
afshin Aug 15, 2022
eba5225
Use for...of instead of each in signaling
afshin Aug 15, 2022
688cb0f
Use for...of instead of each() in messaging
afshin Aug 15, 2022
ad8e19b
In almost all cases, prefer native for..of or .forEach to Lumino util…
afshin Aug 15, 2022
00e6855
Fix examples
afshin Aug 15, 2022
b9a0b1b
Fine.
afshin Aug 15, 2022
0f747de
Add iteration notes to migration.md
afshin Aug 16, 2022
9caca56
Add test for take() where count=0
afshin Aug 16, 2022
d74f6c2
Remove duplicate repeat() test
afshin Aug 16, 2022
929b6d9
Better label for stride() test
afshin Aug 16, 2022
1fbfcae
Better labels for zip() tests
afshin Aug 16, 2022
bc2b48f
Clean up test labels as per review comments
afshin Aug 16, 2022
84ec11b
Change rangeLength signature back
afshin Aug 16, 2022
1442576
Reimplement toObject() using iterables and toArray() as a deprecated …
afshin Aug 16, 2022
da91b28
Better toObject() test
afshin Aug 16, 2022
c6673e8
Fix logic bug in take() that @vidartf noticed
afshin Aug 16, 2022
071756a
Fix typo
afshin Aug 16, 2022
1197d76
Another .forEach() => for...of
afshin Aug 16, 2022
c340bd6
Update packages/algorithm/src/iter.ts
afshin Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add iteration notes to migration.md
@afshin
afshin committed Aug 16, 2022
commit 0f747deda940e54b8faef6ecf6f44dfebe78fd4f
1 change: 1 addition & 0 deletions docs/source/index.rst
View file
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@ Lumino
changelog
api
examples
migration

Indices and tables
==================
142 changes: 142 additions & 0 deletions docs/source/migration.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
# Migration guide for Lumino 1 to Lumino 2

## Iterables, iterators, and generators

### Overview and suggestions on iteration

These are some guiding principles that came up over the course of refactoring the way we handle iteration in Lumino. They are not hard and fast rules, rather they are intuitions that arose while implementing the changes Lumino 2.

### Use `each(...)` sparingly

Iterating through an iterable `bar: Iterable<T>` using native `for...of`, e.g., `for (const foo of bar) {...}` is almost always a better option than using `each(bar, foo => ...)`. Some exceptions to this are:

- When you need the index of an item during iteration and you are _not_ iterating through an array, which already provides the index as a second parameter to a function passed into `.forEach(...)`
- When you could use the array `.forEach(...)` method but you want to be able to terminate iteration early, which you cannot do natively, but can with `each(...)` by returning `false`

Nearly all invocations of `each(...)` have been removed in Lumino 2 (except for tests). See, for example, [this commit](https://github.com/jupyterlab/lumino/pull/346/commits/efb1e919bb359192caeedb726e16ec42d17b3b0f).

### Use `[].forEach(...)` sparingly

Now that we support native ES6 iteration, `for (const value of someArray) {...}` should be favored over `someArray.forEach(...)` because it will not require a context shift every time it invokes the function being applied.

### Use `[Symbol.iterator]()` sparingly

Unless you need a handle on multiple iterators simultaneously (e.g., the way `zip(...)` is implemented in Lumino 2) or you need to hold on to multiple values of your iterable during iteration (e.g., the way we need both the first and the second value of an iterable to implement `reduce(...)` in Lumino 2), most of the time you can simply use `for...of` to iterate through any object that has a `Symbol.iterator` method without invoking that method.

In many places where the Lumino `iter()` utility function has been replaced in Lumino 2 it is not replaced with an invocation of the new `Symbol.iterator` method.

### Use `Array.from(...)` sparingly

`toArray(...)` has been removed. You may be tempted to swap in `Array.from(...)` when you update your code. This _will_ work, but if you simply need to iterate through an iterable, you can use `for...of` directly on the iterable object. This is more performant both in terms of CPU and memory than allocating and populating new `Array` instance before iteration.

If you need a snapshot of every item in your iterable as an array, then `Array.from(...)` is an appropriate replacement for `toArray(...)`.

## Public API changes

### `@lumino/algorithm`

All of the iterator utilities have been changed to use native generators and iterators.

| | `export` | name | note |
| --- | ----------- | ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| ❌ | `type` | `IterableOrArrayLike<T>` | Switch to [`Iterable<T>`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_iterable_protocol) |
| ❌ | `interface` | `IIterable<T>` | Switch to `Iterable<T>` |
| ❌ | `interface` | `IIterator<T>` | Switch to [`Iterator<T>`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_iterator_protocol) / `IterableIterator<T>` |
| ❌ | `function` | `iter<T>(...)` | Switch to [`Symbol.iterator`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/iterator) and [`function*`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*) |
| ❌ | `function` | `iterFn<T>(...)` | Switch to `function*` |
| ❌ | `function` | `iterItems<T>(...)` | We aren't using this function anywhere |
| ❌ | `function` | `iterKeys<T>(...)` | Switch to [`for ... in`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in) |
| ❌ | `function` | `iterValues<T>(...)` | Switch to [`for ... of`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of) |
| ❌ | `function` | `toArray<T>(...)` | Switch to [`Array.from(...)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from) |
| ❌ | `function` | `toObject(...)` | We aren't using this function anywhere |
| ❌ | `class` | `ArrayIterator<T>` | Switch to `[][Symbol.iterator]()` |
| ❌ | `class` | `ChainIterator<T>` | Previous implementation of `chain<T>()` |
| ❌ | `class` | `EmptyIterator<T>` | Previous implementation of `empty<T>()` |
| ❌ | `class` | `EnumerateIterator<T>` | Previous implementation of `enumerate<T>()` |
| ❌ | `class` | `FilterIterator<T>` | Previous implementation of `filter<T>()` |
| ❌ | `class` | `FnIterator<T>` | Switch to [generators](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator) |
| ❌ | `class` | `ItemIterator<T>` | We aren't using this class anywhere |
| ❌ | `class` | `KeyIterator` | Switch to `for ... in` |
| ❌ | `class` | `MapIterator<T>` | Previous implementation of `map<T>()` |
| ❌ | `class` | `RangeIterator<T>` | Previous implementation of `range()` |
| ❌ | `class` | `RetroIterator<T>` | Previous implementation of `retro<T>()` |
| ❌ | `class` | `StrideIterator<T>` | Previous implementation of `stride<T>()` |
| ❌ | `class` | `TakeIterator<T>` | Previous implementation of `take<T>()` |
| ❌ | `class` | `ValueIterator<T>` | Switch to `for ... of` |
| ❌ | `class` | `ZipIterator<T>` | Previous implementation of `zip<T>()` |
| ✅ | `function` | `chain<T>(...)` | Reimplement with native types |
| ✅ | `function` | `each<T>(...)` | Reimplement with native types |
| ✅ | `function` | `empty<T>(...)` | Reimplement with native types |
| ✅ | `function` | `enumerate<T>(...)` | Reimplement with native types |
| ✅ | `function` | `every<T>(...)` | Reimplement with native types |
| ✅ | `function` | `filter<T>(...)` | Reimplement with native types |
| ✅ | `function` | `find<T>(...)` | Reimplement with native types |
| ✅ | `function` | `findIndex<T>(...)` | Reimplement with native types |
| ✅ | `function` | `map<T>(...)` | Reimplement with native types |
| ✅ | `function` | `max<T>(...)` | Reimplement with native types |
| ✅ | `function` | `min<T>(...)` | Reimplement with native types |
| ✅ | `function` | `minmax<T>(...)` | Support native types |
| ✅ | `function` | `reduce<T>(...)` | Support native types |
| ✅ | `function` | `range(...)` | Reimplement with native types |
| ✅ | `function` | `retro<T>(...)` | Reimplement with native types |
| ✅ | `function` | `some<T>(...)` | Reimplement with native types |
| ✅ | `function` | `stride<T>(...)` | Reimplement with native types |
| ✅ | `function` | `take<T>(...)` | Reimplement with native types |
| ✅ | `function` | `topologicSort<T>(...)` | Support native types |
| ✅ | `function` | `zip<T>(...)` | Reimplement with native types |

### `@lumino/collections`

`LinkedList` has been updated to accept native iterables and return native iterators.

| | `export` | name | note |
| --- | ---------- | --------------------------------- | ---------------------------------------------------- |
| ❌ | `class` | `LinkedList.ForwardValueIterator` | Switch to `LinkedList#[Symbol.iterator]` |
| ❌ | `class` | `LinkedList.RetroValueIterator` | Previous implementation of `LinkedList#retro()` |
| ❌ | `class` | `LinkedList.ForwardNodeIterator` | Previous implementation of `LinkedList#nodes()` |
| ❌ | `class` | `LinkedList.RetroNodeIterator` | Previous implementation of `LinkedList#retroNodes()` |
| ❌ | `method` | `LinkedList#iter()` | Switch to `LinkedList#[Symbol.iterator]` |
| ✅ | `function` | `LinkedList.from<T>(...)` | Accept `Iterable<T>` |
| ✅ | `method` | `LinkedList#assign(...)` | Accept `Iterable<T>` |
| ✅ | `method` | `LinkedList#nodes()` | Return `IterableIterator<LinkedList.INode<T>>` |
| ✅ | `method` | `LinkedList#retro()` | Return `IterableIterator<T>` |
| ✅ | `method` | `LinkedList#retroNodes()` | Return `IterableIterator<LinkedList.INode<T>>` |

### `@lumino/datagrid`

`DataGrid` selections are now native iterators.

| | `export` | name | note |
| --- | -------- | ---------------------------------- | --------------------------------------------------- |
| ✅ | `method` | `BasicSelectionModel#selections()` | Return `IterableIterator<SelectionModel.Selection>` |
| ✅ | `method` | `SelectionModel#selections()` | Return `IterableIterator<SelectionModel.Selection>` |

### `@lumino/disposable`

Helper functions for `DisposableSet` and `ObservableDisposableSet` have been udpated.

| | `export` | name | note |
| --- | ---------- | ----------------------------------- | ------------------------------ |
| ✅ | `function` | `DisposableSet.from(...)` | Accept `Iterable<IDisposable>` |
| ✅ | `function` | `ObservableDisposableSet.from(...)` | Accept `Iterable<IDisposable>` |

### `@lumino/widgets`

`Layout` and its sub-classes now use native iterators, e.g. `implements Iterable<Widget>`.

| | `export` | name | note |
| --- | -------- | ------------------------------ | --------------------------------------------- |
| ❌ | `method` | `DockLayout#iter()` | Switch to `DockLayout#[Symbol.iterator]` |
| ❌ | `method` | `GridLayout#iter()` | Switch to `GridLayout#[Symbol.iterator]` |
| ❌ | `method` | `Layout#iter()` | Switch to `Layout#[Symbol.iterator]` |
| ❌ | `method` | `PanelLayout#iter()` | Switch to `PanelLayout#[Symbol.iterator]` |
| ❌ | `method` | `SingletonLayout#iter()` | Switch to `SingletonLayout#[Symbol.iterator]` |
| ✅ | `method` | `DockLayout#handles()` | Return `IterableIterator<HTMLDivElement>` |
| ✅ | `method` | `DockLayout#selectedWidgets()` | Return `IterableIterator<Widget>` |
| ✅ | `method` | `DockLayout#tabBars()` | Return `IterableIterator<TabBar<Widget>>` |
| ✅ | `method` | `DockLayout#widgets()` | Return `IterableIterator<Widget>` |
| ✅ | `method` | `DockPanel#handles()` | Return `IterableIterator<HTMLDivElement>` |
| ✅ | `method` | `DockPanel#selectedWidgets()` | Return `IterableIterator<Widget>` |
| ✅ | `method` | `DockPanel#tabBars()` | Return `IterableIterator<TabBar<Widget>>` |
| ✅ | `method` | `DockPanel#widgets()` | Return `IterableIterator<Widget>` |
| ✅ | `method` | `Widget#children()` | Return `IterableIterator<Widget>` |