TanStack / table

🤖 Headless UI for building powerful tables & datagrids for TS/JS - React-Table, Vue-Table, Solid-Table, Svelte-Table
https://tanstack.com/table
MIT License
25.03k stars 3.07k forks source link

feat(angular-table): adds Angular adapter for tanstack/table (#5326) #5432

Closed KevinVandy closed 4 months ago

KevinVandy commented 6 months ago

nx-cloud[bot] commented 6 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 529bc29f169208a2100f18146239d6629eb62d1b. 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 target - [`nx affected --targets=test:format,test:sherif,test:knip,test:lib,test:types,build --parallel=3`](https://cloud.nx.app/runs/5clWAunCBz?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.

KevinVandy commented 6 months ago

@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:

  1. Convert the angular adapter to go all in on Angular 17 signals. After discussion with @crutchcorn, we've decided that we should only support modern angular going forward, in order to simplify support, and provide the best performance for complicated data-grid features. The min supported version will probably be v17.3 for TypeScript purposes.
  2. Rewrite the examples. The current examples in this PR seem bloated. Let's get the dependencies and configs to the bare minimum, and maybe consider using Vite like all the other examples (though this doesn't matter that much). Also, encourage the use of signals for state.
  3. Write the Table State (Angular) docs with accurate information and syntax.
  4. Get the dependencies, devDependencies, and peerDependencies in check for the adapter and the examples.
  5. Bonus: Create sorting, filtering, and pagination examples since those are somewhat essential for those seeing TanStack Table for the first time.

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.

riccardoperra commented 6 months ago

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:

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 flexRender

KevinVandy commented 6 months ago

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

crutchcorn commented 6 months ago

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!!

riccardoperra commented 6 months ago

Hi, I've added something here 😄 https://github.com/TanStack/table/pull/5442

riccardoperra commented 6 months ago

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"

KevinVandy commented 5 months ago

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.

KevinVandy commented 5 months ago

closing in favor of #5518 for now

KevinVandy commented 5 months ago

@riccardoperra In the future, can you name your branch something other than feat-angular-table. It keeps causing conflicts for which origin I'm trying to push to. Now all your stuff is here for some reason. It's fine, I guess

KevinVandy commented 5 months ago

@riccardoperra I'm looking to clean up the example directories. Do the angular.json files need all of that stuff in it? It at least looks like cli analytics ids should be removed. And I'll probably remove the .vscode folders.

riccardoperra commented 5 months ago

@KevinVandy since there are no test or i18n, i think you can remove the lines for test and extract-i18n

Screenshot 2024-05-05 alle 18 17 45

Also, we can set the budgets to an empty array to avoid future build errors (i've removed it already in all examples that are using fakerjs)

Screenshot 2024-05-05 alle 18 18 41

Yes, cli-analytics should be set to false in order to save the option. Don't remove the line otherwise the builder will ask you on every ng start if you want to add the analytics

  "cli": {
-     "analytics": "...",
+    "analytics":false
  }
KevinVandy commented 5 months ago

For those seeing the PR for the first time, an example of the filters faceted example in a react to angular translation:

image

KevinVandy commented 5 months ago

I started a thread in the TanStack discord where we can discuss this PR a bit more: https://discord.com/channels/719702312431386674/1003326939899113492/1236741193619083355

e-oz commented 5 months ago

Hi, @riccardoperra!

After looking at your code examples, I noticed a couple of things that could be improved. I have a lot of experience with Angular Signals (and Angular in general), so I hope my hints won't be just annoying ;)

In general, your idea is correct: templates should declare what signals they need, and services/components' code should provide them, most of the time as computed(). This approach ensures that your code will work great not only with the OnPush strategy but also with Zoneless Angular apps.

One small detail you could improve here, and your code wouldn't need effect() (and related imperative modifications). In function params:

export function createAngularTable<TData extends RowData>(
  options: () => TableOptions<TData>,
)

you could request not a function, but a signal. Based on this signal, you could create a few computed() signals and return them to the user. This way, whenever options are updated, the whole table is updated.

Memoization can also work for you here, not against you, if it's possible to accurately check if options were actually changed. You could achieve this using a custom equality function. Another "rule of thumb": every signal should be created only once. If code recreates a signal, then something has gone very wrong. I use "readonly" in every signal definition to ensure it never happens.

I'll leave a link to my article about the "template-first" approach with signals here. The article is free, has no ads, and doesn't promote anything. I only included it because I have examples there that could explain the idea better. Feel free to remove my comment if you think this link should not be here.

KevinVandy commented 5 months ago

Thanks for the feedback @e-oz. By the way, this branch is in the TanStack table repo, so those with the time and energy can PR to this PR (feat-angular-table branch). We have some proposed changes in another PR to this branch that has some discussion going back and forth about injectors that can use some expertise too. Feedback on all PRs is highly valued.

riccardoperra commented 5 months ago

Hey @e-oz, thanks for your feedback! I read your article and I find the approach right, but I have some doubts about applying it in this adapter.

I hope I understood your suggestion, but the table instance from @tanstack/table-core depends on:

The full computed approach is fine if you're controlling the state of every portion of the table, but what happens if you need to listen the changes of a value that you don't actually control?

Consider the case of a table where you may want to update the column pinning state without actually store it in a signal. Once a column is pinned, we should apply a conditional style

@Component({})
class MyComponent {
  readonly options = computed(() => ({
    columns: [],
    data: [],
    initialState: {
      columnPinning: {
        left: [],
        right: []
      }
    }
  }));

  readonly table = createAngularTable(options)

  readonly columnPinningState = computed(() => {
    // How I would listen to every columnPinning state?
    return;
  });

  // programmatically pin a table column
  pinColumnLeft(column: Column<unknown, unknown>) {
    column.pin('left');
  }
}
e-oz commented 5 months ago

@riccardoperra in your example, let's modify a few things to make the change of column pinning reactive:

@Component({ standalone: true, selector: 'aaa-aaa', template: '' })
export class MyComponent {
  private readonly columnPinning = signal({
    left: [],
    right: []
  });

  private readonly tableData = signal([]);
  private readonly columns = signal([]);

  readonly options = computed(() => ({
    columns: this.columns(),
    data: this.tableData(),
    columnPinning: this.columnPinning()
  }));

  readonly table = createAngularTable(this.options);

  // programmatically pin a table column
  pinColumnLeft(column: Column<unknown, unknown>) {
    this.columnPinning.update((p) => ({
      ...p,
      left: [...p.left, column]
    }));
  }
}

Side-note: terms that mention time, like initialState, are fine in observables, but signals do not have a time axis - they should be ready to re-compute their value any moment like it's a first time, and they should not know if it's a first time or not - a function we pass to computed() should be pure. Only effect() can work with impure functions.


I'm writing it not to argue, just to explain my suggestion.

riccardoperra commented 5 months ago

I now understood better what you mean. This follow an unidirectional data flow which I used to do mostly using rxjs, but I suppose that this library should allow to have bi-directionality and handle both controlled/non-controlled state.

Doing like you explained to me works for both cases, since once the table is initializated through createAngularTable, your given options get merged and you get a new object which contains more data than you pass, preserving the controlled state. initialState is considered only the first time as I remember.

Consider also that () => TableOptions<T> is currently a valid signature for Signal<TableOptions>, so doing createAngularTable(computed(() => ...)) already works in this version of the library, but underneath we have still have to create a computed with the necessary options

https://github.com/TanStack/table/pull/5432/files#diff-fdf7ef401abdb9233ace51c6aaedc37094bbe7796e6b17df55353deaf4d41a2bR36-R45

The main reason I did in that way was related to something I related some months ago into the @tanstack/angular-query discussion from @eneajaho (https://github.com/TanStack/query/discussions/6293#discussioncomment-7461129)

Anyway, looking at the source code of some specific features (e.g. ColumnVisibility or Grouping), the updater method doesn't only update the state toggling the column id, but in these case also do something else. Without using these methods you'll have to write them by yourself which may not be desiderated.

The current documentation for the table state also suggest to use on[YourFeature]Change or onStateChange to update the controlled state while using the table instance methods.

I would modify your example in something like this

@Component({ standalone: true, selector: 'aaa-aaa', template: '' })
export class MyComponent {
  private readonly columnPinning = signal({
    left: [],
    right: []
  });

  private readonly tableData = signal([]);
  private readonly columns = signal([]);

  readonly options = computed<TableOptions<T>>>(() => ({
    columns: this.columns(),
    data: this.tableData(),
    columnPinning: this.columnPinning(),
    state: { 
       columnPinning: this.columnPinning()
    },
    onColumnPinningChange: (updater) => {
      typeof updater === 'function' 
        ? this.columnPinning.update(updater) 
        : this.columnPinning.set(updater)
    }
  }));

  readonly table = createAngularTable(this.options);

  // programmatically pin a table column
  pinColumnLeft(column: Column<unknown, unknown>) {
    column.pin('left')
  }
}

The pin method will call the onColumnPinningChange which is responsible to update your signal

I'm writing it not to argue, just to explain my suggestion.

No problem at all, it's always nice to have these discussions!

KevinVandy commented 5 months ago

This PR is getting close to shipping. We have enough examples to start out with. The 2 angular specific docs pages for the adapter and table state need to be accurate. I'll do my best to update the syntax to be more accurate to the examples, but more context and explanation about the signals implementation would be highly valued here. @riccardoperra From the convos I've seen here, you may be best to add more docs there to make sure devs use the Angular signals correctly.

Also @riccardoperra @merto20 @e-oz , do we still have any disagreements on either the adapter or the examples using the adapter anymore, or have all of them been resolved?

merto20 commented 5 months ago

@KevinVandy i'm good.

@riccardoperra let me know if I can help out on the documentation. Let's us ship this asap.

e-oz commented 5 months ago

@KevinVandy no disagreements at all.

KevinVandy commented 5 months ago

@merto20 No need to ask for permission to make improvements. Just make the PR @riccardoperra I rewrote the adapter and table state docs for angular based off your examples. Only thing I left a TODO in was for how to create fully controlled state. Guessing it would require computed just like the adapter does it.

merto20 commented 5 months ago

@merto20 No need to ask for permission to make improvements. Just make the PR

@riccardoperra I rewrote the adapter and table state docs for angular based off your examples. Only thing I left a TODO in was for how to create fully controlled state. Guessing it would require computed just like the adapter does it.

@KevinVandy will do.

fully controlled state should also work using input, signal, and model or any signal based types.

KevinVandy commented 5 months ago

As far as I'm concerned, there are just 2 more things to take care of this weekend before shipping.

  1. I'm having @arnoud-dv advise on some build config stuff, and he also offered to review the PR more
  2. Document Angular Signals and Table State a bit more in the table-state docs.
arnoud-dv commented 4 months ago
  1. Rewrite the examples. The current examples in this PR seem bloated. Let's get the dependencies and configs to the bare minimum, and maybe consider using Vite like all the other examples (though this doesn't matter that much). Also, encourage the use of signals for state.

The examples for Query were in Vite before but I converted them to Angular CLI for two reasons:

I would recommend staying on CLI, also the Angular CLI uses Vite internally

riccardoperra commented 4 months ago

I've added some pr for documentation here:

We may have to discuss about this case before releasing the library

arnoud-dv commented 4 months ago

@riccardoperra are the examples working correctly while developing the adapter? We've had trouble in Query before as there were two package.json files, one in the src directory and one in the build directory. It resulted in somewhat random errors where you'd had to run pnpm i and pnpm build until it got in some working (but still unreliable) state. Also when publishing the Angular query package for the first time we got an error. We fixed that in this commit: https://github.com/TanStack/query/commit/4e8a6c5accb4ab7bdf889388d7f7c57e1c47d43a

riccardoperra commented 4 months ago

@arnoud-dv yes, in my environment examples seems working fine while developing, but i always call the build command manually after I made a change. If using watch, we should have thedeleteDestPath set to false (already updated). I remember about the error you mentioned but I didn't had to do in this repo 🤔 Could be something related to the nx cache

Also when publishing the Angular query package for the first time we got an error. We fixed that in this commit:

I forgot about the replicated package.json. Currently I think it will be broken, we have to fix it.

In my company we always published via changesets the /dist folder (through a configuration in the package.json) but we have to remove through script the .npmignore generated file and sync the package.json version. We had a lot of sub-entry so doing it manually in the root package.json was a waste of time.

Looking at the commit, you simply add the exports manually and publish the "root" folder, right? Are you also publishing the source code?

arnoud-dv commented 4 months ago

Looking at the commit, you simply add the exports manually and publish the "root" folder, right? Are you also publishing the source code?

Yes, but I did recently remove the source code from the published package. The published files are determined using files in package.json.

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

arnoud-dv commented 4 months ago

I remember about the error you mentioned but I didn't had to do in this repo 🤔 Could be something related to the nx cache

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.

arnoud-dv commented 4 months ago

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 ❤️

KevinVandy commented 4 months ago

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.

KevinVandy commented 4 months ago

@arnoud-dv

Also when publishing the Angular query package for the first time we got an error. We fixed that in this commit: https://github.com/TanStack/query/commit/4e8a6c5accb4ab7bdf889388d7f7c57e1c47d43a

Looks like we actually still have some inconsistencies in our package package.json, especially with "files". Thanks for linking that query PR.

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.

image
arnoud-dv commented 4 months ago

Looks like we actually still have some inconsistencies in our package package.json, especially with "files". Thanks for linking that query PR.

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.

We're also esm only right now. Wonder if this needs more consideration.

That's correct, Angular packages are ESM-only.

KevinVandy commented 4 months ago

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 Chances are that there are still some issues that will need to be ironed out, so any help that you are able to give this week will be very appreciated.

KevinVandy commented 4 months ago

@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

image
merto20 commented 4 months ago

@KevinVandy just got back after my vacation. This was fixed by @riccardoperra already. I will check what I can do on the 'flexRenderer'.

merto20 commented 4 months ago

@KevinVandy I did some improvement on the flex-renderer code, pls check this PR https://github.com/TanStack/table/pull/5566