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.
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.
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.
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.
You can always create a type util that does that for you inline I guess
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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 filemakePathSimplerslightly to be more type friendlypath.tsto make code more type-friendly (e.g. TPathSegmentInfo)PathDatatype to the more specificTSimplePathDatatype 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.