-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
chore(TS): Path util typings and refactoring #8787
Conversation
# Conflicts: # src/brushes/pencil_brush.class.ts # src/shapes/path.class.ts # src/shapes/text.class.ts # src/typedefs.ts # src/util/misc/misc.ts
# Conflicts: # src/brushes/PatternBrush.ts # src/brushes/PencilBrush.ts # src/shapes/Path.ts # src/shapes/Text/Text.ts # src/typedefs.ts # src/util/path/path.ts
# Conflicts: # src/shapes/Path.ts # src/typedefs.ts
hi @Lazauya ll try to be quick and effective with this PR. I have a couple of things to merge the next hours and then i ll give an eye to this one |
@ShaMan123 @asturur This PR only substantially changes one file (and adds another). There is a minor functionality change: lowercase z is not supported. This was to make the code simpler and more uniform. Other than that, all functionality should remain. |
src/util/path/path.ts
Outdated
current[1] += x; | ||
current[2] += y; | ||
// falls through | ||
case 'L': | ||
x = current[1]; | ||
y = current[2]; | ||
converted = ['L', current[1], current[2]]; |
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.
i have read through the changes of makePathSimpler
and i want to ask you what makes you prefer this converted array approach vs the mutation of the previous array.
Is more verbose.
current was a new array every command so is not a mutation issue. What is it?
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.
if we left it alone then it complains about not being able to assign 'm' to 'M' and things of that nature, which meant that either we'd need a ton of ugly casting or just plain ts-ignores. You can see the approach with types "just slapped on" more or less in the second commit. I think the current one is the most elegant and easiest to read/understand
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.
I don't understand this bit but casting at the top to something loose and then recasting at the bottom to something strict should do the job w/o too much ugliness
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.
You can always create a type util that does that for you inline I guess
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 at least reuse the named variables where possible.
converted = ['L', x, y]
and there are other cases like this down the file
This is added code for nothing but shouldn't be much and most of it is minifiable
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.
Yes in general i don't like the idea of changing code because TS doesn't like it.
TS main purpose is to help avoid errors, it helps with discovery too, but we shouldn't restrict JS at what TS understand, at least i didn't sign up for that when i decided to try a conversion.
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.
IDK I'm obviously biased but I still think that my refactoring is easier to follow. Also I'd like to point out that the refactored function is the same number of lines.
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.
Rejoining
I do prefer reconstructing over mutation if nothing is damaging
And here it is much readable IMO
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.
So I went ahead and made it use the vars instead of current[n]
where possible. I'm still adamant that I think this refactoring is better than doing casting, especially since it's the same number of lines. I agree that sometimes TS is overbearing but I think it's perfectly reasonable in THIS particular case
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.
So I think this is the last open conversation. Is this a blocker? Or are we good to go?
Even exacluding the typecheck regexes i get
Now bundle aside i want to make a real point. Example, all of those becomes - export const reArcCommand = new RegExp(
+ export const reArcCommand =
String.raw`(A) (?:${p} ${p} ${p} ([01]) ?([01]) ${p} ${p} ?)+`,
- 'gi'
- ); and in the path code, in the only place where we use for (const match of pathString.matchAll(rePathCommand)) { we do const rePathCommandExp = new Regexp(rePathCommand)
for (const match of pathString.matchAll(rePathCommandExp)) { thoughts? |
The only real blocker so far is that node 14 doens't like |
I think that makes a lot of sense and I agree with the change. I'll look into |
@Lazauya let me know when you think the code is done, that i will give another read. |
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.
I would love hearing about CD/DX, how is the app? Was it easy to set up and use?
@Lazauya i had a busy week, i ll get back here asap. |
Things in this PR:
path
) that includes a type file and a functions filemakePathSimpler
slightly to be more type friendlypath.ts
to make code more type-friendly (e.g. TPathSegmentInfo)PathData
type to the more specificTSimplePathData
type elsewhere in the codeRationale:
Properly typing the path parsing is a step towards a fully typed SVG parser. I also want to have real-time feedback for invalid/valid path commands. I had this particular issue when using the DefinitelyTyped library; not only were the signatures for path-related things screwed up, but the path command types were very unhelpful.