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

Use native iterators instead of Lumino iterators #346

Merged
merged 62 commits into from
Aug 17, 2022

Conversation

afshin
Copy link
Member

@afshin afshin commented Aug 10, 2022

This PR refactors Lumino packages to switch to iterators and generators (Mozilla (MDN), TypeScript) instead of custom Lumino iterators. Fixes #333.

Iteration suggestions for Lumino users

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 in 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.

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 deprecated. 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>
interface IIterable<T> Switch to Iterable<T>
interface IIterator<T> Switch to Iterator<T> / IterableIterator<T>
function iter<T>(...) Switch to Symbol.iterator and function*
function iterFn<T>(...) Switch to function*
function iterItems<T>(...) We aren't using this function anywhere
function iterKeys<T>(...) Switch to for ... in
function iterValues<T>(...) Switch to for ... of
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
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 toArray<T>(...) @deprecated, use Array.from(...) or for ... of
function toObject(...) 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>

@afshin afshin added the maintenance Dependencies, build, technical debt, etc. label Aug 10, 2022
@afshin afshin added this to the Lumino 2 milestone Aug 10, 2022
@afshin afshin self-assigned this Aug 10, 2022
@afshin afshin force-pushed the native-iterators branch 6 times, most recently from 349ac0c to 8c3b2c0 Compare August 11, 2022 15:19
@afshin afshin marked this pull request as ready for review August 11, 2022 20:27
@afshin
Copy link
Member Author

afshin commented Aug 12, 2022

cc: @jasongrout @vidartf @ibdafna @mwcraig — apropos our conversation about Lumino 2's impact on @jupyter-widgets/* packages on the Jupyter Widgets weekly call.

@afshin afshin changed the title Remove Lumino iterators in favor of native iterators Use native iterators instead of Lumino iterators Aug 13, 2022
@afshin afshin force-pushed the native-iterators branch 4 times, most recently from 89f9a77 to 0da98e4 Compare August 15, 2022 07:59
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Nice work, thank you!

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

thanks a lot for this @afshin

Would you mind adding a migration.rst file in the documentation that include your very useful advices from the this PR description.

@afshin
Copy link
Member Author

afshin commented Aug 16, 2022

I added migration.md (because the docs apparently can be .rst or .md) but I'm still not sure the docs are building/working properly. I'd propose leaving that to fix in:

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

Good work here! I tried my best to review the code in the actual algorithm package vs behavior changes, and made some comments w.r.t that. Also added some nitpicks since I was already looking 😄

As a maintainer of a few library packages dependent on Lumino, my main question for Lumino v2 will be "how hard will it be for my code-base to support both v1 and v2 during the transition period?". The fewer conditionals I need on version or contorted imports that will be needed, the better managed the upgrade will be. In that regards, I think there might be value in keeping the toArray and toObject functions around as deprecated convenience functions. AFAICT, there should be little maintenance cost in keeping them for a cycle, while it should bring high benefit to anyone that wants that ^1 || ^2 dependency.

packages/algorithm/src/find.ts Show resolved Hide resolved
packages/algorithm/src/find.ts Show resolved Hide resolved
packages/algorithm/src/find.ts Show resolved Hide resolved
packages/algorithm/src/range.ts Outdated Show resolved Hide resolved
packages/algorithm/src/take.ts Outdated Show resolved Hide resolved
packages/algorithm/tests/src/repeat.spec.ts Outdated Show resolved Hide resolved
packages/algorithm/tests/src/retro.spec.ts Outdated Show resolved Hide resolved
packages/algorithm/tests/src/stride.spec.ts Outdated Show resolved Hide resolved
packages/algorithm/tests/src/take.spec.ts Outdated Show resolved Hide resolved
packages/algorithm/tests/src/zip.spec.ts Outdated Show resolved Hide resolved
@afshin
Copy link
Member Author

afshin commented Aug 16, 2022

As a maintainer of a few library packages dependent on Lumino, my main question for Lumino v2 will be "how hard will it be for my code-base to support both v1 and v2 during the transition period?". The fewer conditionals I need on version or contorted imports that will be needed, the better managed the upgrade will be. In that regards, I think there might be value in keeping the toArray and toObject functions around as deprecated convenience functions. AFAICT, there should be little maintenance cost in keeping them for a cycle, while it should bring high benefit to anyone that wants that ^1 || ^2 dependency.

Because the types have changed, even a convenience toArray and toObject won't stop you from having to rewrite code. I strongly advocate for removing these, but I'm prepared to be outvoted on this 🙂

@jasongrout
Copy link
Contributor

Because the types have changed, even a convenience toArray and toObject won't stop you from having to rewrite code. I strongly advocate for removing these, but I'm prepared to be outvoted on this 🙂

If I have code toArray(luminoiterator), how would I need to rewrite code? Before luminoiterator was a lumino iterator, now it is a js iterator, but either way I get an array in the end, without having to change my code, right?

@afshin
Copy link
Member Author

afshin commented Aug 16, 2022

If I have code toArray(luminoiterator), how would I need to rewrite code? Before luminoiterator was a lumino iterator, now it is a js iterator, but either way I get an array in the end, without having to change my code, right?

If your luminoiterator is now a native iterable by virtue of something upstream having changed its types, then I would have some suggestions for how you should change your code:

  1. Do you really need for it to be an array? In many cases, you should probably use for ... of
  2. If you truly need it to be an array, use Array.from(luminoiterator) (with the understanding that luminoiterator is no longer a Lumino iterator)

@jasongrout
Copy link
Contributor

jasongrout commented Aug 16, 2022

for how you should change your code:

Right - I thought what @vidartf was proposing was deprecated functions that allowed you to do ^1||^2 without changing code, without too much burden on lumino maintainers (i.e., keep toArray since it is used often). I'm +1 on easing the transition with a deprecation cycle for common functions.

@afshin
Copy link
Member Author

afshin commented Aug 16, 2022

@vidartf, @jasongrout, @davidbrochart, two things:

  1. I reimplemented toObject(...) even though we weren't using it because it's pretty straightforward and doesn't hurt.
  2. I re-added toArray with a @deprecated tag.

Co-authored-by: Vidar Tonaas Fauske <vidartf@gmail.com>
@blink1073
Copy link
Contributor

Let's merge and...iterate.

@blink1073 blink1073 merged commit 4e9ff8e into jupyterlab:main Aug 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintenance Dependencies, build, technical debt, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Lumino iterators in favor of native iterators
5 participants