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

Improve TypeScript types #112

Closed

Conversation

Methuselah96
Copy link
Contributor

@Methuselah96 Methuselah96 commented Aug 2, 2023

There are some short-comings in the types as they currently exist, namely:

  1. When using GUI.add, there is no type-checking to make sure that property is a key of the object.
  2. When using GUI.add with an array or object of options for the third argument, there is no type-checking to make sure the options are assignable to the property's value.
  3. There is no type-checking of the callback passed to Controller.onChange and also therefore the parameter for the callback that is passed to Controller.onChange is not inferred, since onChange accepts any function.

I wanted to go and create a draft PR to get some initial feedback since I believe this is treading new territory. Most of the existing JSDoc comments are fairly standardized, these would be the first JSDoc comments that are really targeted toward making the type definitions better. It seems like both linting and API creation don't like this new direction.

Is this something you're open to? Or is there a alternative, preferred way to improving the type definitions?

@Methuselah96 Methuselah96 changed the title Improve type for onChange Improve types Aug 2, 2023
@Methuselah96
Copy link
Contributor Author

Note that there are additional changes that are worth making (namely adding overloads to GUI.add to map the right argument types to the right controller) which I don't think will be possible using JSDoc as a basis for types. Is creating a separate type definitions without it being based on JSDoc be something you'd be open to? Or maybe there's a way to modify the types after they're generated for the cases that JSDoc types can't handle?

@georgealways
Copy link
Owner

Hi there, thanks for this!

On a high level, I'd like to restrict the JSDoc annotations to standard tags, and the .d.ts should be a generated artefact. JSDoc is used in this library to help generate the documentation page, and to provide hinting for editors that support it. I see the fact that we can generate a .d.ts from this as well to be a bonus.

Using @template like this is going to confuse the API doc generation scripts. I'm also curious if VSCode can make use of these annotations when using vanilla JS (being able to verify the existence of a key on an object before runtime seems like a TypeScript thing?).

I'd be open to more specificity in type checking if we can do it without major changes to the docs generation or the creation of separate type definitions.

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Aug 2, 2023

I'm also curious if VSCode can make use of these annotations when using vanilla JS (being able to verify the existence of a key on an object before runtime seems like a TypeScript thing?).

I think it would only be useful for TypeScript users, I don't think VSCode will do anything with the annotations when using vanilla JS.

I'd be open to more specificity in type checking if we can do it without major changes to the docs generation or the creation of separate type definitions.

Okay, so that is a "yes" or a "no" to using @template tags for TypeScript generics? I don't think any of these changes will work without generics. Or is there another path forward for making these TypeScript improvements?

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Aug 2, 2023

Another option would be to move the type definitions to DefinitelyTyped (and make a @types/lil-gui package). That way TypeScript users could improve the types without worrying about muddying up the JSDoc or docs generation and you wouldn't have to worry about it.

@Methuselah96 Methuselah96 changed the title Improve types Improve TypeScript types Aug 2, 2023
@georgealways
Copy link
Owner

I'm not totally against using @template if it improves the experience for TypeScript users, but only if it:

  1. doesn't break scripts/api.js and the API page
  2. doesn't degrade the dev experience for JS users

The hand-maintenance / generation of types in a @types/ repository feels a little brittle to me.

I'm all in favor if we can do this while still hitting the requirements above. But if not, then I would have to say that yes, this is a limitation of using a JS library in TypeScript.

@Methuselah96
Copy link
Contributor Author

Thanks for your consideration and timely response.

At this point I'm leaning toward just making these types improvements in @types/three (which is my impetus for working on this). I'll probably use a patch file to just patch the parts of the definition files that I'm making changes to in order to make sure I'm keeping up with any changes to the declaration file from here.

While hand-maintenance of the types may feel brittle, it seems like the only option for the type improvements I'm planning to make. I would love for other users of lil-gui outside of @types/three to benefit from these type changes, let me know if you're ever interested in including these changes here instead of just in @types/three.

@Methuselah96 Methuselah96 deleted the better-on-change-types branch August 2, 2023 14:16
@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Aug 2, 2023

My only other thought is:

But if not, then I would have to say that yes, this is a limitation of using a JS library in TypeScript.

If you're planning to just be a JS library, it seems worth considering not shipping TypeScript type definitions and let that be handled by the TypeScript community on DefinitelyTyped.

@georgealways
Copy link
Owner

All good! Yes, I'm curious about this idea of "patching" the type definitions that can't be described easily in JSDoc. Could you tell me a bit more about what that looks like? Thanks!

@Methuselah96
Copy link
Contributor Author

Yeah, let me play around with that a little bit and I'll create a draft PR showing what that could look like.

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.

None yet

2 participants