-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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-table): adds Angular adapter for tanstack/table (#5326) #5432
Conversation
* Angular adapter for tanstack table initial * build with: ng packagr * Add angular examples basic grouping * Add angular examples basic grouping * Update angular examples basic grouping * Add angular example selection * regen lock file * package upgrades, angular table docs * prettier * move around some deps * removed unused dependency from angular package * fix deps --------- Co-authored-by: Kevin Van Cott <[email protected]>
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 529bc29. 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 1 targetSent with 💌 from NxCloud. |
@imaksp @danielglejzner @crutchcorn @jrgokavalsa @Lord-AY @JeanMeche @tonivj5 Work continues on this branch for the TanStack Angular Table adapter. Anyone can make a PR to this branch instead of just code reviewing the original PR. Things that need to be done:
After these 4-5 things are accomplished, we'll feel good about shipping this. I would appreciate help with these tasks, as Angular is the framework I am least familiar with. |
Hi, I'm currently working on a design system that will use angular w/ tanstack table. These days I already had to work on my own on an adapter that use signals, I'm following you to understand if I can directly integrate the official one, or keep ours anyway. I still have some doubts about my current implementation, I tried these approaches:
const table = createAngularTable(() => ({
...
});
// re-run automatically on every change
table().options.state
table().getHeaderGroups()
table().getRowModel()
...
const table = createAngularTable(() => ({
...
});
table.options().state // `options` transformed from plain property to computed
table.getHeaderGroups() // This is a computed
table.getRowModel() // This is a computed
table.set... // Still same property
... One problematic thing I'm trying to solve is supporting onPush change detection everywhere, so with the first approach I think it should be ok, but only if you don't memorize the signal or if you create a new reference of the object. The second one, however, could be riskier as I'm not sure that all the properties must be transformed into computed in the same way, or there is a risk that the table will not update correctly due to extra memoization. This is what I'm using at the moment, hope it can help and if you can give me some advice 😄 export function createAngularTable<TData extends RowData>(
options: () => TableOptions<TData>,
) {
const injector = inject(Injector);
// Supports required signal input (https://angular.io/errors/NG0950)
return lazySignalInitializer(() => {
return runInInjectionContext(injector, () => {
const resolvedOptionsSignal = computed<TableOptionsResolved<TData>>(
() => {
return {
state: {},
onStateChange: () => {},
renderFallbackValue: null,
...options(),
};
},
);
const table = createTable(resolvedOptionsSignal());
const tableSignal = signal(table);
const state = signal(table.initialState);
function updateOptions() {
const tableState = state();
const resolvedOptions = resolvedOptionsSignal();
table.setOptions(prev => ({
...prev,
...resolvedOptions,
state: {...tableState, ...resolvedOptions.state},
onStateChange: updater => {
if (updater instanceof Function) {
state.update(updater);
} else {
state.set(updater);
}
},
}));
// Spreading this otherwise signal with default `equals` will not trigger on state change
// Another solution could be using `equal: () => false` on `lazySignalInitializer` and `tableSignal`
untracked(() => tableSignal.set({...table}));
}
updateOptions();
// Currently I'm using this to avoid updating options twice the first time.
let skipUpdate = true;
effect(() => {
void [state(), resolvedOptionsSignal()];
if (skipUpdate) {
skipUpdate = false;
return;
}
untracked(() => updateOptions());
});
return table;
});
});
} One another important thing about
|
@riccardoperra, your described situation makes you sound like a god-send. lol I'm not against totally re-implementing the adapter part of this PR if needed. Not that the original PR was bad, but signals are just going to be much better if we're requiring Angular 17+ anyway. I'd love @crutchcorn to review your adapter code, because my angular knowledge is limited. The initial state stuff looks good though. You can make a PR to the branch that this PR is made from. If the adapter is changed, get at least 1 example using state (row selection) updated too if you could. We don't need 100% comprehensive PRs yet. |
I'll willingly admit that I haven't spent as much time in Angular signal land as you have @riccardoperra but your code LGTM I'd merge it (assuming tests passed) Let's start a PR to keep track of your changes and we can do more minutia code review and testing. And, of course, thank you so much for your help!! |
packages/angular-table/src/index.ts
Outdated
} | ||
} | ||
|
||
renderComponent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this method supposed to be public ?
packages/angular-table/src/index.ts
Outdated
|
||
renderComponent() { | ||
this.vcr.clear() | ||
if (!this._flexRender) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This early return seem redundant with the following conditions
Hi, I've added something here 😄 #5442 |
Hi @KevinVandy, I'm answering you for this, sorry for the late reply. Unfortunately I had to pause the work on our table in our design system due to some priority needs. I accidentally pushed a few things that broke the pr examples and the build, which I can fix soon. Anyway, The current integration here is the latest one we have in our ds which seems working fine. I would like to get some feedbacks from someone who works with angular to understand if what I did with the proxy approach is ok. Regardless, the returned object is a signal so it should work fine. We personally had to memorize some functions to optimize performance with this approach, but I think it's "normal" |
Thinking of closing this PR if we cannot make more progress on it. We need someone who is knowledgeable in Angular to not just advise, but actually make the adapter and examples. |
Yes, but I did recently remove the source code from the published package. The published files are determined using "files": [
"build",
"!**/*.d.ts",
"!**/*.d.ts.map",
"build/rollup.d.ts"
], Note I also now exclude all type declarations except a rolled up one generated by Microsoft API extractor. Flattening type declarations using API extractor is recommend by the Angular package format for good reasons IMO but I see few Angular packages actually do this. If you want I could add a PR next week to do that for Table too? Could be done after a first package publish too. Also I like the API report API extractor generates, it allows to keep oversight of the public API easily. |
Might have been an issue in the pnpm version used back then, pnpm got confused and would also change the package location in lockfile between the two package.json locations constantly. |
Looks good to me, will there be a beta period after publishing where breaking changes could be made if necessary? Very cool to have TanStack Table on Angular too ❤️ |
If breaking changes are needed to fix something in the first few weeks before a ton of people are using this, that's fine. Though we probably won't be giving the packages beta versions, unless we figure out configuration for that. |
* tslib should be a dependency, see: https://angular.io/guide/angular-package-format#tslib * ensure angular examples are run as web container on code sandbox * update lock file --------- Co-authored-by: Kevin Vandy <[email protected]>
* add flexRender documentation * fix docs * fix rendering component section heading
Looks like we actually still have some inconsistencies in our package package.json, especially with Edit: maybe our implementation is ok. We might not find out until shipping... We're also esm only right now. Wonder if this needs more consideration. |
It's not the same as Query but it's correct I'd say. If we want to implement type declaration rollup using API extractor we could change it but it's optional. Edit: there is the potential issue with the duplicate package.json but it seems to work fine so far in Table. If not, the same fix could be applied as in Query.
That's correct, Angular packages are ESM-only. |
I don't think there is much more to review until we try shipping it, so will try to do that shortly. Thanks for all the help so far @arnoud-dv @riccardoperra @merto20 @jrgokavalsa |
return this.renderComponent(content) | ||
} | ||
|
||
private renderStringContent() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be great to explicit the return types.
@merto20 Rendering is acting weird in the grouping example. That's the one you were testing your stuff with, right? fyi @riccardoperra https://tanstack.com/table/latest/docs/framework/angular/examples/grouping |
@KevinVandy just got back after my vacation. This was fixed by @riccardoperra already. I will check what I can do on the 'flexRenderer'. |
@KevinVandy I did some improvement on the |
Angular adapter for tanstack table initial
build with: ng packagr
Add angular examples basic grouping
Add angular examples basic grouping
Update angular examples basic grouping
Add angular example selection
regen lock file
package upgrades, angular table docs
prettier
move around some deps
removed unused dependency from angular package
fix deps