Updating types

[stramel]

So some of this is still broken. I attempted to read through all the API Docs and through most of the source to rid the typings of any and anything missing in the typings.

Looking for feedback on how I have broken this stuff down and ways we could improve it.

I have broken all hooks down to:

• The hook function
• Options
• Hooks
• State
• Column Options
• Instance Props
• Column Props
• HeaderGroup Props
• Row Props
• Cell Props

I'm still iterating on some ways to make the plugin/composable nature easier to work with on a per-table basis.

/cc @ggascoigne

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 67273ca:

Sandbox	Source
strange-fast-kkk42	Configuration

[janhartmann]

Amazing, this is so close! I actually tried myself a few times to create typings for this. But the nature of the API makes it a bit hard. This, however, looks much better than what I could do.

[ggascoigne]

BTW as I'm going through this, I have to say that I love the organization here, this is so much cleaner that what you started with.

[stramel]

Thanks @ggascoigne! I wanted to organize it so we could easily modify and add plugins.

It wouldn't be as accurate but, for now, we could have the main interfaces extend all plugins. Which would give us a working type definition for all plugins. Then after, we could work on making it more configurable based on which plugin(s) you use.

[ggascoigne]

I'll admit that most of my reservations to this come from not using several of the stock plugins and using similar but different in-house ones. It's easy to merge declarations, but I don't know how to un-merge them.

I'm trying to balance my desire to not have to deal with that, vs. wanting to be a good team player :)

We could just put all of the declaration that extends everything in a separate file? Then there's an easy option for those that want it, but it would be easy to ignore for those that don't?

And I'll admit that we might just have to go that route in the long run, I've got no clue how to make this self configuring at this point.

[ggascoigne]

I don't want to push a revision against your branch, that seems rude, but I've been working on using our new types with ported version of a couple of the original examples from this repo.

Specifically, I ported the kitchen sync example to typescript, there's a somewhat modified version of this index.d.ts file at ggascoigne@8ad8a06#diff-b462adda0ffcd9c8cefed0ad03060f25

I'm not sure how much of the changes you want to incorporate, the biggest one was around switching types over to interfaces since you can't do declaration merging on types.

There was also some messiness around filters. What I have works for this example, but I wouldn't say it's the general solution.

[stramel]

@ggascoigne I appreciate all the time you have been taking to review and test these changes.

I took some of the changes you had implemented in the kitchen sink example. However, a few things didn't line up with the documentation or code so I left those out. The main thing I haven't yet changed is switching to interfaces, I plan to do this.

WRT filters, I was very unsure about how those actually functioned. They definitely need cleaned up. I will try to shore those up.

I have an idea for making it generic per instance but it will take some playing around. It would be using generics. Something like:

useTable<MyDataType, UseSortByPlugin>({ columns, data, manualSorting: true }, useSortBy)

where UseSortByPlugin would be a map of all types necessary for the plugin.

type UseSortByPlugin = {
  options: UseSortByOptions,
  hooks: UseSortByHooks,
  state: UseSortByState,
  columnOptions: UseSortByColumnOptions,
  instanceProps: UseSortByInstanceProps,
  columnProps: UseSortByColumnProps,
}

Then the useTable would be able to apply that type to the appropriate place.

type UseTableOptions<D, P1> = {
  columns: UseTableColumnOptions<D> & P1['columnOptions']<D>
  // ...
}

This would obviously make the type definition pretty ugly as we would have to have a minimum of n + 1 generics where n equals the number of plugins. Then if you wanted to use more than the number of plugins that the library offers, you would then have to deal with the nasty-ness of interface merging. But this would hopefully allow you to manage each table's plugins independently instead of globally.

Interested to hear your thoughts. I haven't implemented this, just given it a bit of thought.

[ggascoigne]

That's an interesting idea, and seems like it might be a way forward, and a variant could be used for useTableState which has very similar issues.

Aside from the maintainability issue I'm not too worried about how ugly the types end out being as long as they end out being easy to actually use - of course, that's the trick :(

I wonder if there's some way to derive the types from the list of plugins themselves, but the options available to getting the different associated types are rather limited, so some sort of key type per hook (such as the one in your example) might well be the key to getting it working.

[stramel]

@ggascoigne

I wonder if there's some way to derive the types from the list of plugins themselves

I have had the same thought and have been brainstorming some ways around doing so... Will continue to investigate.

[janhartmann]

@stramel how far off to do you think we are now? I am eager to try this out in public ;-)

[stramel]

@janhartmann I'm actively working on it 😅 I modified it locally on one of my projects and it only had a bit missing. I'm working on checking the types and still playing around with the plugin architecture.

Hope to have it ready to go by Monday if I can find some time this weekend.

[ggascoigne]

@evark Since getTableBodyProps was only added to the api 22 hours ago, I can't say that I'm surprised it's not in the types yet.

As for how to correctly extend types so you can have them match the plugins that you are using, that's what needs writing.

[ggascoigne]

FYI #1564 - a starting place for docs.

[evark]

Thanks @ggascoigne, this is something to start, for removing all these ugly //@ts-ignore lines.
By the way, which is the best method to use a local version of types in source tree? Currently, I have a copy of original index.d.ts as react-table.d.ts in my src folder and in tsconfig.json a line like this: "exclude": ["node_modules/react-table"]

[ggascoigne]

@evark That's pretty much what I do. Once the latest types are merged this will be comes easier, but for now you are on the right path.

[ggascoigne]

@stramel Where are we on this, there seem to be a few unresolved issues still here? Would you like me to open a PR against you fork with a couple of changes to push into this PR? I'm not sure how else to try and help.

[stramel]

@ggascoigne What are the unresolved issues left that we have a consensus on how to move forward with?

[ggascoigne]

@stramel There only two (and I suppose that the key bit there is about consensus :) ).

• UseTableOptions extending Record, this defeats the type checking on creating tables and is easily handled by creating a type for the additional params that you want to pass in to the options. I really care about this since it's very awkward to undo if it's in the standard types.
• getGroupByToggleProps on UseGroupByColumnProps, I agree that this one seems odd
  but suspect that it's a sign that our differentiation between Header and Column is more strict than actually exists in the code. This can always be added after the fact though so whilst I think that this is probably correct it's easy to work around.

Other than these two, these types work in my prod app and in the samples that I've ported to TypeScript.

[tannerlinsley]

There have been changes to the API around useTableState. Please see the latest commits and release to get up to date.

[ggascoigne]

I also noticed that if you don't delete the index.d.ts from the react-table package its types also get used which masks the optional nature of state in the TableInstance. If you do delete node_modules/react-table/index.d.ts you suddenly find that you can't destructure state. I was wondering why I was seeing this and you weren't. I thought that since the types weren't explicitly listed in the package.json file, that they weren't exposed to typescript, that's apparently not the case.

In other words, we should fix that one as well.

[stramel]

@ggascoigne

• UseTableOptions extending Record, this defeats the type checking on creating tables and is easily handled by creating a type for the additional params that you want to pass in to the options. I really care about this since it's very awkward to undo if it's in the standard types.

I'm willing to remove this for now if you will add a caveat/note in your typescript docs PR about extending the UseTableOptions for adding custom options.

• getGroupByToggleProps on UseGroupByColumnProps, I agree that this one seems odd
  but suspect that it's a sign that our differentiation between Header and Column is more strict than actually exists in the code. This can always be added after the fact though so whilst I think that this is probably correct it's easy to work around.

I'm still unsure about this. @tannerlinsley Can you weigh in on this?

[stramel]

This now has the latest changes with

removal:

• useTableState

additions:

• useAbsoluteLayout
• useBlockLayout
• useResizeColumns

Would love to get this merged sooner rather than later so we can have a much more solid base moving forward.

[ggascoigne]

@stramel I'll update the docs, thank you.

[ggascoigne]

This is awesome, and I feel that this is ready to merge.

[stramel]

Whoops! Didn't mean to close this! 😅

@tannerlinsley Do you feel comfortable moving forward with this and allowing us to continue to iterate on this as a base?

[tannerlinsley]

Yep. I think this is a good start. Good to merge?

[stramel]

Yes, I believe everyone is on the same page with getting this merged.

[tannerlinsley]

Released as a new beta! Good work guys. Seriously this is soooo cool.