-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
refactor: TypeScript improvements to packages/mermaid/src/rendering-util/rendering-elements
#5974
refactor: TypeScript improvements to packages/mermaid/src/rendering-util/rendering-elements
#5974
Conversation
Instead of being a `Record<string, ShapeHandler>`, the type now knows every key in the variable.
All of the shapes in `packages/mermaid/src/rendering-util/rendering-elements/shapes/` use `D3.Selection`s as input parameters and as return parameters. Although, for some reason, passing them to roughjs seems to work?
Convert the `packages/mermaid/src/rendering-util/rendering-elements/nodes.js` file to TypeScript at `packages/mermaid/src/rendering-util/rendering-elements/nodes.ts`
Convert the `packages/mermaid/src/rendering-util/rendering-elements/shapes/util.js` file to TypeScript at: `packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts`.
This let's us confirm that the types we're passing to `insertNode()` are valid.
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
The latest updates on your projects. Learn more about Argos notifications ↗︎ Waiting for the first build to start… |
packages/mermaid/src/rendering-util/rendering-elements/shapes/stateStart.ts
Show resolved
Hide resolved
packages/mermaid/src/rendering-util/rendering-elements/shapes/tiltedCylinder.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check on use of types/interface. try to use one.
packages/mermaid/src/rendering-util/rendering-elements/nodes.ts
Outdated
Show resolved
Hide resolved
See: mermaid-js#5974 (comment) See: mermaid-js#5974 (comment) Co-authored-by: saurabhg772244 <[email protected]>
I don't know what exactly the type does or is for, but I've tried to type it to what it seems to be.
I believe @sidharthv96 prefers the use of Line 84 in 3e922c8
|
packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
Outdated
Show resolved
Hide resolved
packages/mermaid/src/rendering-util/rendering-elements/shapes.ts
Outdated
Show resolved
Hide resolved
packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
Outdated
Show resolved
Hide resolved
Handle `NaN` values by using `parseFontSize()`. Suggested-by: mermaid-js#5974 (comment)
Suggested-by: mermaid-js#5974 (comment) Co-authored-by: Sidharth Vinod <[email protected]>
📑 Summary
A bunch of TypeScript improvements to
packages/mermaid/src/rendering-util/rendering-elements
.I'd recommend reviewing this PR commit-by-commit!
packages/mermaid/src/rendering-util/rendering-elements/shapes.ts
: Change the type ofshapes
from a genericRecord<string, ShapeHandler>
to a more specific type that knows every key in the variable, and every function and return value.packages/mermaid/src/rendering-util/rendering-elements/shapes/
: Correct the types of all the shapes in this directory. Most of these functions were incorrectly using a raw DOMSVGAElement
as a parameter. However, they actually accept ad3.Selection<SVGAElement, unknown, Element | null, unknown> | d3.Selection<SVGGElement, unknown, Element | null, unknown>
. I've turned most of these functions into template functions, since otherwise TypeScript has issues with these types.packages/mermaid/src/rendering-util/rendering-elements/nodes.ts
: Convert thenodes.js
file to TypeScript.packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
: Convert theshapes/util.js
file to TypeScript.packages/mermaid-layout-elk/src/rendering-util/rendering-elements/render.ts
: Remove someany
types fromrender()
. This was mainly so I could see what types were being passed tosrc/rendering-util/rendering-elements/shapes/util.ts
so I could accurately type it.📏 Design Decisions
Calling
d3.selection.attr('attr', undefined)
is not supported and throws a TypeScript error https://d3js.org/d3-selection/modifying#selection_attrHowever, I've noticed that it seems to do the same thing as
d3.selection.attr('attr', null)
(e.g. deleting the attribute if it exists), so I've made a new helper function calledhandleUndefinedAttr
that basically just does?? null
:mermaid/packages/mermaid/src/utils.ts
Lines 944 to 954 in 16a5fc0
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.?? 0
inmermaid/packages/mermaid/src/rendering-util/rendering-elements/shapes/util.ts
Line 52 in 16a5fc0