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

Added more specific typing and description LayoutOptions #195

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Jona898
Copy link

@Jona898 Jona898 commented Oct 24, 2022

I used the knownLayoutOptions() to generate more specific typings and description for LayoutOptions.
I tried to parse as much as possible but for example the enums aren't typed.

I don't know, if i placed the file i used to generate the typing at the right place. Feel free to change it. I also have the file in typescript, if that is wanted, but the code base is java-script, so i removed the type annotations.

Added the file i used to generate typing
@Jona898
Copy link
Author

Jona898 commented Oct 24, 2022

I don't know, if there is a possability, that the interface has extra properties. I left a "[key:string]: any" if the interface isn't complete. It it isn't needed you can remove them as well.
Even with this line the code autocompletion is way better

@Jona898
Copy link
Author

Jona898 commented Oct 24, 2022

I tried the typings and some need to have the elk. at the beginning.
I don't know, if all need them or if it hurts, if it is appended to all of them. But it seems to work for the properties I tested

@Jona898
Copy link
Author

Jona898 commented Oct 28, 2022

Sorry for the continuus changes. I now used https://www.eclipse.org/elk/reference.html to add the Enum Types as Union Strings.
I added a few ToDo comments. If you could check these passages i think it should be finished.

@Jona898
Copy link
Author

Jona898 commented Oct 29, 2022

I added al Variants of the EnumSet<...> as String Union Representation.
This adds quite a lot of combinations. It it wanted, that the options can be in any order? If a fixed Order of options is acceptable that would shrink the options for the Set greatly.
To see what I mean look at the changes in last commit.

@uruuru @soerendomroes Any Preferences from you guys?

@soerendomroes
Copy link
Member

How do your changes integrate with the current elkjs build?
Can generating the typings be part of npm run build?

@Jona898
Copy link
Author

Jona898 commented Nov 1, 2022

The Problem with generating the types in the continuous integrating is that elk.knownLayoutOptions() returns not the enum type, but only the value is of type enum. So it will be hard to map these Types together.
grafik
As base of the types I used the current elk-api.d.ts.

Because not all types are string anymore, but "real" numbers or booleans Typescript will mark those as errors in deployment see action run on my repository.

@soerendomroes
Copy link
Member

Sorry for the delay.
These option might change a lot in ELK. If this would require manual effort, I would rather create an "any" type for enums than write 1000 lines of enums by hand, or do you generate them too?

@joryphillips
Copy link

The first thing I did when I decided elkjs was a great solution is I started manually adding the typings I was using.

I think this PR would be a huge help towards improving developer experience! I hope it or one like it can get merged.

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.

3 participants