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(angular-virtual): add Angular adapter #726

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

garrettld
Copy link
Contributor

@garrettld garrettld commented May 10, 2024

Adds an Angular adapter:

  • Supports Angular >=17
  • Signal-based API
  • Packaged with ng-packagr
  • Examples
  • Works with required input signals
  • Works with OnPush change detection

StackBlitz playground: https://stackblitz.com/edit/stackblitz-starters-sd2s4h?file=src%2Frow-virtualizer-fixed.component.ts

@KevinVandy
Copy link
Member

Nice, maybe we'll ship 2 new TanStack Angular adapters this week. The Table one should ship in the next few days.

@garrettld
Copy link
Contributor Author

garrettld commented May 10, 2024

Hi! At this point I'm mostly just looking for some intent to move forward by the project maintainers as well as feedback from the community. I don't necessarily expect this to be merged as-is, though it is fully functional.

This began as a weeknight project inspired by a tweet and before long I had implemented all the examples and fell in love with it. The API and its implementation are heavily inspired by Angular Query and the upcoming Angular Table.

There are some specific areas of the API that I would love community feedback on. For example, the adapter replaces the getScrollElement function on the tanstack/virtual-core Virtualizer with a simple scrollElement property and automatically unwraps ElementRefs for you, which simplifies things a bit, especially when using signal queries:

scrollElement = viewChild<ElementRef<HTMLDivElement>>('scrollElement')

// current implementation
virtualizer = injectVirtualizer(() => ({ scrollElement: this.scrollElement() })

// theoretical implementation that matches the core API
virtualizer = injectVirtualizer(() => ({ getScrollElement: () => this.scrollElement()?.nativeElement })

This is currently the only property from the core Virtualizer API that has been renamed or changed outside of turning things into signals, but there are other parts of the API that could potentially be more signal-ified. For one example, rangeExtractor can create friction when the callback's result depends on one or more signals.

headerIsSticky = input.required<boolean>()
virtualizer = injectVirtualizer(() => ({
  rangeExtractor: (range: { min: number, max: number }) =>
    // oops! this won't be re-ran when `this.headerIsSticky()` changes!
    this.headerIsSticky() && range.min > 0
      ? [0, ...defaultRangeExtractor(range)]
      : defaultRangeExtractor(range)
})

// you have to do this instead:
headerIsSticky = input.required<boolean>()
virtualizer = injectVirtualizer(() => ({
  // rangeExtractor is recreated whenever this.headerIsSticky() changes
  rangeExtractor: this.headerIsSticky()
    ? (range: { min: number, max: number }) => range.min > 0
      ? [0, ...defaultRangeExtractor(range)]
      : defaultRangeExtractor(range)
    : defaultRangeExtractor
})

This exact issue (callbacks that close over signals are not reactive) is present in Angular Query as well, and it's not necessarily a dealbreaker, but I can imagine an API where the options object is static (i.e., it's not recreated), but instead each property on it is (optionally?) a signal.

For the TS wizards, think something like this (simplified):

type AngularVirtualizerOptions = { [K in keyof VirtualizerOptions]: Signal<VirtualizerOptions[K]> }

for example:

virtualizer: injectVirtualizer({ // no callback, just a {}
  // pass signals directly for everything
  data: this.data,
  count: computed(() => this.data().length),
  size: computed(() => this.big() ? ((index) => 250) : ((index) =>  100)),
  // functions accept signal parameters and expect signal return values
  rangeExtractor: (range: Signal<{ min: number; max: number }>) => computed(() =>
    // because `rangeExtractor` must return signal, you don't have to remember to hoist signal invocations out of the callback
    this.headerIsSticky() && range().min > 0
      ? [0, ...defaultRangeExtractor(range)]
      : defaultRangeExtractor(range))
})

The adapter could easily convert this into the shape that the core Virtualizer needs.

Are these terrible ideas? Awesome improvements? If you have thoughts on any of these examples or anything else, please share them!

@garrettld
Copy link
Contributor Author

The table example has been updated to use the TanStack Angular Table 🚀

Copy link

nx-cloud bot commented May 14, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f8fd587. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@Jimmysh
Copy link

Jimmysh commented Jun 4, 2024

Any further updates?

@piecyk
Copy link
Collaborator

piecyk commented Jun 4, 2024

Any further updates?

Will try to find some time this week, as not that up to date with angular, need to go through comments made by @garrettld

@garrettld
Copy link
Contributor Author

I know it's been a while, but I haven't forgotten about this. It looks like it needs a rebase, which I'll try to get done this week.

I've done some more exploration of the API and I'm more and more convinced that passing signals directly (as described in my earlier comment) is less error-prone and a better DX. I hope to update the PR to at least allow for this type of usage so that others can play around with it and give feedback.

I also noticed that the current implementation isn't cleaning up after itself properly, so I'll fix that, too.

Finally, this will break in a future Angular version when/if angular/angular#56501 is merged, because it currently uses { allowSignalWrites: true }, which is apparently going away. Angular's effect and afterNextRender functions used in this PR are in "Developer Preview", meaning they may be changed in minor or patch versions like this. I'm going to add mention of this to the docs.

@aPinix
Copy link

aPinix commented Jul 25, 2024

Are there any updates on this? Thanks

@Jimmysh
Copy link

Jimmysh commented Aug 18, 2024

@garrettld
Any further updates?

@garrettld
Copy link
Contributor Author

garrettld commented Aug 18, 2024

Thanks for the ping. I've cleaned up the commit history and updated to 3.9.0.

The risk of breakage due to reliance on Angular signal APIs that are in Developer Preview is still real, so I've updated the docs to mention that this library is in an "experimental stage" similar to Angular Query. I don't really expect the @tanstack/angular-virtual API to change much, but there might be subtle behavior (timing) changes if the Angular team ends up changing effect scheduling. @piecyk should we rename this to @tanstack/angular-virtual-experimental?

I've been working with this library in a real app and a component library for a few months and I think the API that exists in this PR is in a good state. The more I use it, the more I think that deviating from the core API isn't worth it. Allowing both signal and non-signal versions of the options object just made it more confusing for developers, and I now think that consistency with other framework adapters, with the core API, and with the approach taken in other @tanstack/angular-* libraries is the way to go. Aside from the question above about naming this "experimental", this is ready to merge, IMO.

@KevinVandy
Copy link
Member

I'm not the maintainer of this repo, @piecyk is

@piecyk
Copy link
Collaborator

piecyk commented Aug 19, 2024

Thanks @garrettld for this awesome work, i think we are fine with the angular-virtual. Please sync with main and fix the lock file.

Adds an Angular adapter:

- [x] Supports Angular >=17
- [x] Signal-based API
- [x] Packaged with ng-packagr
- [x] Examples
- [x] Works with required input signals
- [x] Works with OnPush change detection
@garrettld
Copy link
Contributor Author

Please sync with main and fix the lock file.

Done

@piecyk piecyk merged commit 1043bd7 into TanStack:main Aug 19, 2024
3 of 4 checks passed
@aPinix
Copy link

aPinix commented Aug 19, 2024

Awesome! When can we start using it?
Are there docs on how-to?

Thanks guys 👌

@garrettld
Copy link
Contributor Author

@aPinix I have a PR open here to get the docs site updated, but the library is available for use now, and you can follow the quick start guide on npm.

@garrettld garrettld deleted the feat-angular-adapter branch August 23, 2024 20:11
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.

5 participants