-
Notifications
You must be signed in to change notification settings - Fork 138
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
7.x.x feature Autogenerated TS Types #192
7.x.x feature Autogenerated TS Types #192
Conversation
- Update types reference in package json to our new types enterance file
Maybe some of the other TS users of this community such as @enijar, @Michaelazzz or @kevinjardine can provide some thoughts on this. For more context, please refer to #189. |
//JSDoc Related Imports
/* eslint-disable no-unused-vars */
import TypographyFont from './TypographyFont';
import MSDFTypographyCharacter from './msdf/MSDFTypographyCharacter';
import InlineCharacter from './InlineCharacter';
/* eslint-enable no-unused-vars */ That's far from being perfect, but at least it identifies as what it aims. Preventing some to remove it. I think that would also be three-shaked during build. Let's wait thoughts from typescripters, but if
Im okay with your proposal |
Yeah from what I understood, your original goal was to make this as hands off as possible. The more I researched how other communities do this, the more I found that a more pure TS solution would require more maintenance and hands on updates as APIs are refactored. Defining |
I've been reading further about using JSDoc for existing JS libraries and while its possible, I think the biggest thing that is hindering the use of JSDoc generated types is our inability right now to generate types that indicate how these classes inherit from one another. For example: A ThreeMeshUI Block "inherits" from I will continue to read up on using JSDoc for more pure JS projects in my free time and document further thoughts here, or should I document my thoughts over in #189 ? Right now maybe we shouldn't merge this until some of these more open ended issues are resolved... |
In the middle/long term, it won't matter. Those mixins are going to dismantle themself progressively. Did you check If we know we are making progress in the right direction, but are still blocked by some culprit, we may start by this jsdoc PR. And then check at some intervals if the right time has come. |
Sounds good on the mixins going away. Probably a step in the right direction. I will see if the Overall I think you are right though, this might be a solution that will completed as the library evolves and is completed in chunks. On a side note, should this PR commit the new build files as they do change when I run build? I noticed PRs of lately have committed those files. |
Sorry, have had a busy week here. The extends directive would help in a more traditional fashion, but won't play nicely with the autogenerated docs from what I am seeing here.
If you are ok with this, I will mark this PR as ready for review again. Looks like I will need to update the branch we are merging into since you have merged the font class branch since then. One thing though is I will update the PR to have |
Thank you @JoeCoppola-HIA, Im totally fine with your suggestion 👍 |
Thanks for the merge, excited to be getting closer to a usable solution! |
Overview
The PR in its current state addresses the compiler issues laid out here #189 (comment).
I fought a lot with this trying to find the best approach to incorporating types in the JSDocs parameters without adding too much clutter, while also keeping it more vanilla JS like. The solution of importing the modules referenced in param comments but not directly used in code seems like a middle ground solution. It isn't perfect but trying to go a more pure TS solution might be cumbersome to main contributors of the library.
Failed Import Directive Approach
I attempted to make a centralized typedefs file and define them all there, and then use the jsdoc import directive, but that in turn made it even more confusing to follow and ultimately went against the whole goal here. There are several outstanding tickets within the TS community to make this less of a headache, but it doesn't seem to be making much progress at the moment.
results in
Current PR Solution
Importing the types the way I do works, but without disabling the
no-unused-vars
checks for the imports, webpack will throw a ton of warnings that I don't think we would want.This PR also updates the out directory for the types to be in a dedicated
types
folder, and updates the package json to point to the entry *.d.ts file.Outstanding Issues
There are still some additional issues when I tried to use these type files in our main project. One major issue is we use
ThreeMeshUI.MeshUIComponent
all over to generalize Three Mesh UI Components like Text, Blocks, and InlineBlocks since it acts like a base class for these components. But theMeshUIComponent.js
file exportsMeshUIComponent
as a function that returns a local class calledMeshUIComponent
. I'm currently unsure how to address this on our side, or on the three mesh UI side to make our code happy. I am open to ideas there, ultimately being able to refer to a base class for all component types would be preferred.Another issue with the current types I have yet to figure out is this error spit out by our build system when trying to use the new types generated:
Any thoughts there would help.
Final Thoughts
Lastly, I did not commit the types in their current form, yet at least. I am unsure as to how you guys go about the release process, and if we wanted to commit it or build it each time before publishing. Let me know how you would like to proceed there.
Thank you, I believe we are one step closer to having useful TS Types, but understand if this needs to be discussed further overall.