vincjo / datatables

A toolkit for creating datatable components with Svelte
https://vincjo.fr/datatables
MIT License
372 stars 15 forks source link

[Suggestion] Improve types #31

Closed jst-r closed 1 year ago

jst-r commented 1 year ago

Summary

Currently the library liberally uses any, Function and string types. There is nothing wrong with doing it this way, however the developer experience could be meaningfully improved by implementing generics.

I've made a slice of what the implementation might look like in my fork. My vscode formatted it to 2 space indent, so it might look ugly, sorry for that.

What the actual improvement is?

User land code in both examples is the same, only $lib/core has been changed.

With current implementation the data type is any and keys are string. Because of that we get no type checking and autocomplete. image

After adding some generics we get both. image

How does that work?

Since the DataHandler instance gets passed around anyway, it makes sense to attach the type to it.

export default class DataHandler<T extends { [key: string]: unknown } = any>

With that adapting Th component to use the type is pretty straightforward.

<script lang="ts">
  type T = $$Generic<{ [key: string]: unknown }>;
  type OrderBy<Row> = keyof Row | ((row: Row) => Row[keyof Row]);

  import type { DataHandler } from '$lib/core';
  export let handler: DataHandler<T>;
  export let orderBy: OrderBy<T>;
  export let align: 'left' | 'right' | 'center' = 'left';
  const identifier = orderBy ? orderBy.toString() : orderBy;

  const sorted = handler.getSorted();
</script>

What's next?

It shouldn't be that hard to extend the same idea to the rest of the library, and I have plentiful free time right now, so I can implement the changes. But that's still a big change and I wanted to discuss it here first.

@vincjo As the author an maintainer of the library, do you feel like the library would benefit from stricter types (like ones in the example above)?

If you have any questions - feel free to ask. And thank you for the awesome work on the library.

vincjo commented 1 year ago

Thank you for these clear explanations. It seems very relevant to me.

I looked at your fork, didn't see any particular restraint (thx for the preview :ok_hand:).

I'm currently quite limited when it comes to Typescript so this would be a great help

jst-r commented 1 year ago

I'm done with a draft of implementation, please check it out in the fork.

I couldn't figure out how to make formatting from npm run format match what you have in the repo to make a nice-looking PR. Can you help me with that, please?

Changes follow the same idea as in the example above. Filter and sorting types are a bit convoluted right now, but it's fixable.

What's left now is cleaning up the code, updating the docs, and testing.

Also narrowing most of the exposed types is a pretty big breaking change. Since there are only minor runtime changes (only additional type checks iirc), most of the user land code should be fine. But it is still important to figure out what the migration will look like.

huntabyte commented 1 year ago

Hey @vincjo , great looking project you have here! Probably the cleanest and most straightforward I've seen for Svelte. Is there any plans to release these type improvements? I'd like to use this as the DataTable for shadcn-svelte, but not having those types is a dealbreaker.

Let me know if this is in the plan or if I should start a fork!

Thank you!

vincjo commented 1 year ago

Hey @huntabyte Everything seems ok for an integration.

@jst-r i'm ready to receive your PR It seems to me that everything is working correctly. Once again, thx for your help

About code formatting, i'll just add a semi: false in .prettierrc, it'll be fine!

jst-r commented 1 year ago

Sorry for making you wait. Here's the PR. Docs are in progress right now

vincjo commented 1 year ago

@jst-r :pray:

Just realeased npm package 1.9.0

I refactored here and there, now type declarations live in $lib/index.ts