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

feat(types): improve partial-autocomplete and type inference #265

Merged
merged 7 commits into from
Dec 6, 2023

Conversation

M0hammedImran
Copy link

@M0hammedImran M0hammedImran commented Nov 22, 2023

Summary

Update types with partial-autocomplete and value type for better type inference.

Description for the changelog

Update types.ts with (string & {}) union.
Other info

@tbouffard
Copy link
Member

This is a rework of #256 that looks promising.
I hope to have time to take a look at it before next Wednesday. If I haven't reviewed it by then, feel free to ping me in the comments.
😸

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M0hammedImran Thanks for your contribution.

I directly committed to the branch to introduce StyleArrayValue and StyleShapeValue types, as the ArrayValue and the ShapeValue types are for built-in/core values provided by maxGraph according to the JSDoc.
I'm not very happy with the names of the new types, so if you have better names, please suggest them. 😄

packages/core/src/types.ts Outdated Show resolved Hide resolved
@tbouffard tbouffard added the enhancement New feature or request label Dec 3, 2023
@M0hammedImran
Copy link
Author

M0hammedImran commented Dec 4, 2023

@tbouffard, I have noticed an inconsistency in JSDoc and type here types.ts.

If I read the JSdoc the allowed type is boolean but the actual type used a ENUM DIRECTION.
Please help me understand.

Mohammed Imran added 2 commits December 4, 2023 16:36
- Use Record utility type where every necessary.
- Define EdgeParametersValue.
- Use EdgeParametersValue for insertEdge.
@M0hammedImran
Copy link
Author

@tbouffard, I have also noticed there are enums in Constants files. I think we can change it to Object with as const and reduce redundant type.
For example, the DIRECTION enum in Constants.ts can become.

export const DIRECTION = {
  NORTH: 'north',
  SOUTH: 'south',
  EAST: 'east',
  WEST: 'west',
} as const;

And, DirectionValue in types.ts will become.

type Direction = typeof DIRECTION;
export type DirectionValue = Direction[keyof Direction];

The type would remain the same.
image

Of course, I also realize that there are other files where we might have to make some changes, but I think it's worth it.
Let me know what you think of this.

@tbouffard
Copy link
Member

@M0hammedImran thanks for the update.

About DIRECTION, yes this is something that could be changed but this is a larger subject as it applies to a lot of enum. See #205.
I suggest to handle this in a dedicated PR to be able to first merge the work already done here.
We should probably discuss about the enum in #205 first to find a convenient solution. Related fixes/improvements will probably introduce breaking changes.
What do you think about this way of doing?

@tbouffard
Copy link
Member

tbouffard commented Dec 4, 2023

@tbouffard, I have noticed an inconsistency in JSDoc and type here types.ts.

If I read the JSdoc the allowed type is boolean but the actual type used a ENUM DIRECTION. Please help me understand.

Yes this seems to be a error introduced during the migration from mxGraph to maxGraph. This has probably been introduced in b51504d when the style configuration switched from a string to a dedicated object.

In mxGraph, this was a boolean: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/util/mxUtils.js#L2250
I suggest to fix it in a dedicated PR as this will introduce a breaking change and we need to check if it is used in Stories.

@M0hammedImran
Copy link
Author

@M0hammedImran thanks for the update.

About DIRECTION, yes this is something that could be changed but this is a larger subject as it applies to a lot of enum. See #205. I suggest to handle this in a dedicated PR to be able to first merge the work already done here. We should probably discuss about the enum in #205 first to find a convenient solution. Related fixes/improvements will probably introduce breaking changes. What do you think about this way of doing?

Yes, I think it would be wise to create a dedicated PR.

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
As discussed in comments, there are other types to improved. This will done later.

@tbouffard tbouffard changed the title Update types with partial-autocomplete and value type for better type inference. feat(types): improve partial-autocomplete and type inference Dec 6, 2023
@tbouffard tbouffard merged commit 8d6727a into maxGraph:development Dec 6, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants