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

Provide a more reactive IterableDiffer #18178

Open
EricABC opened this Issue Jul 17, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@EricABC

EricABC commented Jul 17, 2017

I'm submitting a...


[ X ] Feature request

What is the motivation / use case for changing the behavior?

DefaultIterableDiffers provides a brute-force implementation for iterables that is optimal for small arrays where the change information is lost. It's clearly the best choice for most common scenarios. However, often in more declarative applications the change information is known. For example, I might dispatch an ngrx action to add an item to a list. Rather than maintaining my own copy of the list state, I just want to feed ngFor the change. NgFor performance would be associated only to the size of changes now, instead of the list size. Although it could be added separately, it has that 'OnPush' declarative feel that seems at home in the core angular culture.

The implementation would be substantially simpler than scanning arrays for changes, something like this crude untested example that could be upgraded in a PR...

<ng-container *ngFor="let item of changes | async">...</ng-container>

... where changes is an Observable<Iterable<IterableChangeRecord<V>>>

/** Assumes immutable iterable */
export class ExplicitDiffer<V> implements IterableDiffer<V> {
    private last?: NgIterable<IterableChangeRecord<V>>;

    /**
     * @todo NgForOf implies external state is maintained by requiring NgIterable here.  Nothing in ngFor actually iterates.
     */
    diff(object: NgIterable<V>): IterableChanges<V> | null {
        let current = <Iterable<IterableChangeRecord<V>>><any>object;
        return this.last === current ? null : new ExplicitChanges<V>(this.last = current);
    }
}

export class ExplicitChanges<V> implements IterableChanges<V>, Iterable<V> {
    /**
     * Leave both current and previous indices unset to indicate an item change.
     * @todo Equal current and previous index.
     * @todo Optimize by sorting.
    */
    constructor(private changes: Iterable<IterableChangeRecord<V>>) {
    }

    forEachOperation(fn: (record: IterableChangeRecord<V>, previousIndex: number, currentIndex: number) => void): void {
        // Instead of tracking moves in a large collection (default differs), we modify position of each change in a small collection.
        for (let record of this.changes) {
            if (record.previousIndex == null && record.currentIndex == null)
                continue;
            if (record.previousIndex == null) {
                for (let change of this.changes)
                    if (change.previousIndex && change.previousIndex > <number>record.currentIndex)
                        (<number>change.previousIndex)++;
            }
            else if (record.currentIndex == null) {
                for (let change of this.changes)
                    if (change.previousIndex && change.previousIndex > record.previousIndex)
                        (<number>change.previousIndex)--;
            }
        }
        for (let record of this.changes)
            fn(record, <number>record.previousIndex, <number>record.currentIndex);
    }

    forEachIdentityChange(fn: (record: IterableChangeRecord<V>) => void): void {
        for (let record of this.changes) {
            if (record.previousIndex == null && record.currentIndex == null)
                fn(record);
        }
    }

Other thoughts: An array of changes could just use mergeMap to change things one at a time. but that is sounding like a different directive than ngFor that doesn't use differs at all. Perhaps that's an even better approach since it's not even 'for'-ing over an iterable.

@EricABC

This comment has been minimized.

EricABC commented Jul 17, 2017

It's not entirely as trivial to implement (async pipe eats events since it only cares about present value, ability to not map index/count to avoid list iteration, etc...)
But here is a working plunkr as a separate directive without using differs at all.

@Toxicable

This comment has been minimized.

Contributor

Toxicable commented Jul 18, 2017

So just to sum it up and make sure I understand what you're proposing, it's basically an "OnPush" version of NgForOf ? Which requires a different IterableDiffer implementation.

@trotyl

This comment has been minimized.

Contributor

trotyl commented Jul 18, 2017

I guess this should also combine with allow extends Differs at application level proposal.

@EricABC

This comment has been minimized.

EricABC commented Jul 18, 2017

@Toxicable Philisophically its related. Default ChangeDetectionStrategy is to NgFor as OnPush strategy is to this proposal. NgZone and change detection in the default strategy is there to discover all the mysteries that imperative work is doing in the application. When you switch to redux-based approaches (ie. ngrx), you could probably simplify or even remove NgZone and build a truly event-driven app, opening performance by orders of magnitude (I learned this talking to angular folks at ngConf, and looking at the PRs between you guys it seems like I'm preaching to the choir). DefaultIterableDiffers is much like NgZone in that it discovers all the possible imperative changes to something. With this proposal, I just want to directly tell NgFor those changes to apply.

In my specific use-case where I first wanted this capability, I use directives to tie ngrx selectors to the DOM dynamically in a hierarchy (dynamic lazy-loading layouts). When dynamic layout state comes in, I want to keep the imperative work down to simply dispatching an action, 'InsertChildTileInDashboardAction' or something. With <tile *ngFor=...> my action reducer will have to lose the change information and manipulate an array of tiles that I have to maintain in state. Then ngFor has to scan the array for changes, undoing the loss of information. If ngFor could accept deltas instead of full arrays, my state is simply maintained in the view and in BehaviorSubjects inside ngrx. I never really need to know about the full array.

@trotyl Ya, I saw a few tickets about this. I think its already possible except for a bug in that you cant do it in the module provider. #11309 for example. I haven't had too much issue with this, I can extend the interfaces just fine and then simply store it on the list of factories in a module constructor as a workaround. Nothing else in angular prevents me from implementing either approach (new directive or custom differs). However, I am arguing that angular should consider this as a native part of the library.


If there is any stomach for it, then the first question is which approach is better?

  • Separate differs that uses existing ngFor:

    • Pro: Very easy to add in with no breaking changes
    • Con: The 'diff' function signature would be a bit inaccurate (its not really an iterable of T. It's not really and 'Iterable'-Differs at all).
    • Con: User needs to be aware of a special class on which you push changes.
      • This flies in the face of immutable components, unless I'm holding on to special change arrays in my state, and that doesn't feel very much like state.
  • Separate directive, common base class, or improved ngFor:

    • Pro: Allow users to keep things immutable. The only thing I need to do is wire up observables.
    • Pro: NgFor could get a good cleanup, separating notions of Differs, identity (trackBy), and view manipulation.
    • Con: Could be substantial changes to ngFor if its not a separate directive. Either way there is alot of common code between accepting iterables or accepting deltas. However, it probably doesn't affect the public NgFor API.
    • Con: Observable must be directly supplied as an input, or a new pipe must be created. This is because the async pipe will just overwrite the latest value with each event. If two events come in sequence, it will only see the last and never see a change.
  • Pros for either:

    • Declarative from the initial imperative action through the UI.
    • Performance is now indexed to change rate, not array size (technically only if you disable the count and index).
    • Optionally, changes could be managed one at a time. In other words, the data source doesn't have to think through how to match current and previous indices in an aggregate set of changes, so long as the user can supply changes in order.
      • A new pipe is no longer sufficient for this case, otherwise its only pushing out one change per frame.
  • Cons for either:

    • We never got rid of aggregating a set of changes. We had to hold on to a set of changes coming in between change detection calls (even with OnPush). The user must be aware that supplying changes is different from supplying values.

Personally, I'm leaning toward a separate directive or improved ngFor. That's the approach I took with the plunkr, a little different from the original differs suggestion.


One final thing: There is a third option that is much more drastic (but I asked someone at ngConf and they said it had been discussed). Pipes deal with modifying values during an explicit call to transform. What about transforming input and output bindings themselves? In other words,
<tag (click | debounce:50)="onClick()">.
This problem could be solved by allowing input transforms as well:
<tag [input | onImmediateScheduler | delta]="someObservable".


If there is any desire to implement anything here, let me know. I can PR if desired. If it doesn't belong in angular or is much more long term, I can put this up on @sharpangles if desired.

Sorry if this spams you, but i think its pertinent to a few folks I've run across: @robwormald @brandonroberts @MikeRyanDev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment