chore(TS): Parse transform attribute typing #8878

Lazauya · 2023-04-30T00:38:47Z

Some minor typing to move towards a fully-typed parser.
Includes some minor logic changes, meaning simpler regex and smarter parsing.
Removed the superfluous numberRegExStr constant and spruced up the reNum type.

…r-types

src/parser/parseTransformAttribute.ts

asturur · 2023-04-30T07:00:24Z

Seems good so me apart that minor comment.

…r-types

…to parse-transform-attr-types

Lazauya · 2023-04-30T23:01:53Z

OK @asturur should be ready now.

asturur · 2023-05-01T10:12:17Z

Good to me, let's wait @ShaMan123 too.

asturur · 2023-05-01T10:21:01Z

src/parser/parseTransformAttribute.ts

+  attributeValue = attributeValue
+    .replace(new RegExp(`(${reNum})`, 'gi'), ' $1 ')
+    // replace annoying commas and arbitrary whitespace with single spaces
+    .replace(/,/gi, ' ')
+    .replace(/\s+/gi, ' ')


actually, sorry, this part here is duplicated in path. Do we want to do a function util, under utils/internals that clean up the string? so we can also test that the input and output of those 3 asserts combined are always what we want.

it can totally be called removeExtraSpaceAndCommasFromSVGAttributes it doesn't have to be generic or anything like that.

If it is used more than once, sure

Even if used once

BTW why use 2 regexp replace instead of combining the rexep into one with an or command?

i don't know, it could be because we replace first the comma with a 'space' and only then we can clean up multiple spaces. Probably this is safer to follow and a single regexp is a bit more hard to read and be sure it does what it has to do.

asturur · 2023-05-01T10:26:29Z

src/parser/parseTransformAttribute.ts

+    const operation = matchedParams[1];
+    const args = matchedParams.slice(2).map(parseFloat);


Suggested change

const operation = matchedParams[1];

const args = matchedParams.slice(2).map(parseFloat);

const [ _, operation, ...rawArgs ] = matchedParams;

Would this work?

then

const args = rawArgs.map((arg) => parseFloat(arg));

There is a risk using parseFloat inside a map like that, that we are calling parseFloat as

(value, index, wholeArray) => parseFloat(value, index, wholeArray)

And you never know what happens

I agree,
Should call parseFloat with no additional params
Regarding spreading, whatever

asturur · 2023-05-01T10:30:06Z

src/parser/parseTransformAttribute.ts

-      operation = m[1],
-      args = m.slice(2).map(parseFloat);
+  for (const match of attributeValue.matchAll(reTransform)) {
+    const transformMatch = new RegExp(transform).exec(match[0]);


i never remember how regexp work, in this case can we create the regexp before the loop and just use it for each match[0]?

Dido.
It never does what I want it to.
And mdn says one thing while chrome does another from what I remember

asturur · 2023-05-01T10:30:41Z

Sorry i got some additional minor comments, feel free to comment if you think some are unnecessary

ShaMan123

Regexp can be great if you know how to.
For me it takes time to trial and error so I don't like it but appreciate the power.
So I can't really review those parts. Unless I dive into it.
My main comment is naming or the signature of the parsing methods. Read more where I commented.
And another thing about combining replacers.

ShaMan123 · 2023-05-01T12:39:49Z

src/parser/scaleMatrix.ts

+ */
+type TMatScale = [x: number] | [x: number, y: number];
+
+export function scaleMatrix(matrix: TMat2D, args: TMatScale | number[]) {


Is there a way to return a matrix from these methods instead of passing one and mutating it?
Better because of isolation

At least naming should imply it
setScaleFromSVGMatrix or something

So i think the code is very old and bad.
I would argue we fix it in a followup since @Lazauya made a work on types and regex and i think we can focus on that.
But yes, definetely this matrix loop is wrong, it has a mutable array that gets renewed evey loop with a new one, in a let. Is just bad code.

ShaMan123 · 2023-05-01T12:41:37Z

src/typedefs.ts

+export type TMat2D = [
+  a: number,
+  b: number,
+  c: number,
+  d: number,
+  e: number,
+  f: number


ShaMan123 · 2023-05-01T12:45:44Z

src/parser/parseTransformAttribute.ts

+  attributeValue = attributeValue
+    .replace(new RegExp(`(${reNum})`, 'gi'), ' $1 ')
+    // replace annoying commas and arbitrary whitespace with single spaces
+    .replace(/,/gi, ' ')
+    .replace(/\s+/gi, ' ')


BTW why use 2 regexp replace instead of combining the rexep into one with an or command?

asturur · 2023-05-03T20:03:49Z

@Lazauya i want to publish a new beta i m merging this and following up myself for the minor changes requested

Lazauya added 7 commits April 28, 2023 19:09

transform attribute types

eb91390

Fixes to types

695876a

Merge remote-tracking branch 'origin/master' into parse-transform-att…

3688fb5

…r-types

fix regex and logix

9dc2789

prettier

f9209c7

changelog

abb95fc

Merge remote-tracking branch 'origin/master' into parse-transform-att…

36fbbf0

…r-types

asturur reviewed Apr 30, 2023

View reviewed changes

src/parser/parseTransformAttribute.ts Outdated Show resolved Hide resolved

asturur and others added 4 commits April 30, 2023 13:00

Merge branch 'master' into parse-transform-attr-types

73984b8

remove unnecessary string.raw

4c34c58

Merge remote-tracking branch 'origin/master' into parse-transform-att…

ddfc6e1

…r-types

Merge remote-tracking branch 'laz-fork/parse-transform-attr-types' in…

8277205

…to parse-transform-attr-types

asturur approved these changes May 1, 2023

View reviewed changes

asturur reviewed May 1, 2023

View reviewed changes

ShaMan123 reviewed May 1, 2023

View reviewed changes

asturur merged commit dc644fb into fabricjs:master May 3, 2023

asturur mentioned this pull request May 4, 2023

Bump version up to beta4 #8882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(TS): Parse transform attribute typing #8878

chore(TS): Parse transform attribute typing #8878

Lazauya commented Apr 30, 2023

asturur commented Apr 30, 2023

Lazauya commented Apr 30, 2023

asturur commented May 1, 2023

asturur May 1, 2023

asturur May 1, 2023

ShaMan123 May 1, 2023

ShaMan123 May 1, 2023

ShaMan123 May 1, 2023

asturur May 1, 2023

asturur May 1, 2023

ShaMan123 May 1, 2023 •

edited

Loading

asturur May 1, 2023

ShaMan123 May 1, 2023 •

edited

Loading

asturur commented May 1, 2023

ShaMan123 left a comment

ShaMan123 May 1, 2023

ShaMan123 May 1, 2023

asturur May 1, 2023

ShaMan123 May 1, 2023

ShaMan123 May 1, 2023

asturur commented May 3, 2023

		const operation = matchedParams[1];
		const args = matchedParams.slice(2).map(parseFloat);

	const operation = matchedParams[1];
	const args = matchedParams.slice(2).map(parseFloat);
	const [ _, operation, ...rawArgs ] = matchedParams;

chore(TS): Parse transform attribute typing #8878

chore(TS): Parse transform attribute typing #8878

Conversation

Lazauya commented Apr 30, 2023

asturur commented Apr 30, 2023

Lazauya commented Apr 30, 2023

asturur commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaMan123 May 1, 2023 • edited Loading

Choose a reason for hiding this comment

asturur commented May 1, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asturur commented May 3, 2023

ShaMan123 May 1, 2023 •

edited

Loading

ShaMan123 May 1, 2023 •

edited

Loading