Add type definitions for svg

[TrySound]

Ref https://github.com/facebook/flow/issues/2332 Used spec https://www.w3.org/TR/SVG/types.html https://www.w3.org/TR/SVG11/coords.html https://www.w3.org/TR/SVG/struct.html This is initial coverage for <svg> element There's a question about previously added SVGMatrix interface. All methods except inverse look irrelevant. @e98cuenc, what is the source of it?

[TrySound]

What am I missing here, guys?

[jgebhardt]

Check out the output of the failed Travis job– the line numbers included in the error messages don't match the spec, since you inserted new declarations throughout the file. You likely need to regenerate the spec fixtures and run the tests locally to ensure they pass.

[koddsson]

@TrySound Are you still interested in landing this? I would like to and was thinking I could work off this PR instead of starting from scratch 😸

[TrySound]

@koddsson Will give your an access to fork. I have not time right now. Hope to join later.

[koddsson]

You likely need to regenerate the spec fixtures and run the tests locally to ensure they pass.

@jgebhardt I've figured out how I can run the tests locally but I didn't find how I can regenerate the spec fixtures locally. Is there a command I can run?

[koddsson]

I read somewhere that I'm supposed to run ./runtests.sh -r bin/flow so I gave that a go. Going to see if I can fix this conflict now and then maybe CI will run green.

[koddsson]

Seems like CI is timing out and failing with the following message:

The job exceeded the maximum time limit for jobs, and has been terminated.

[gabelevi]

It's possible that the new libs are adversely affecting Flow's performance. I can try importing and running our perf tests

[gabelevi]

Yeah, just anecdotally, the tests feel way slower. If I run

flow server --profile tests/promises

With flow v0.57.3, I get

TimingEvent `InitLibs`: start_wall_age: 0.064070; wall_duration: 0.687360; cpu_usage: 0.025481; flow_cpu_usage: 0.017863

But with this PR applied I see

TimingEvent `InitLibs`: start_wall_age: 0.063500; wall_duration: 34.279512; cpu_usage: 0.029731; flow_cpu_usage: 0.017896

So there's some exponential blowup in the new libdefs for some reason.

[koddsson]

@gabelevi Ok that's interesting! I'll dig around and see if I can figure something about this out.

[koddsson]

I tried removing the // @flow comment in lib/dom.js since I thought that may have been the reason this is slow but am still getting a really slow bin. If someone has any ideas, I'm all ears :)

[jstafford]

Took at look at this PR, since I'm starting to use SVGs in my project. Noticed 2 small problems: On line 8 of lib/dom.js the following annotation was added:

// @flow

This seems like the kind of change that is not necessary and could add an unexpected 35 seconds to the tests.

Also, at line 3404 SVGElement is used before it is defined. SVGElement is defined at line 3426, and moving it up to line 3402 didn't cause any other errors. It's only a warning for me, but might still be causing an automated test failure, perhaps.

I hope someone can find time to check these two ideas and see if this fixes this PR. It's be great to have these types in flow, without having to grab them out of this PR.

[koddsson]

@jstafford I made those changes and merged master. I can't build this locally for some reason so I can't verify it working. Let's see what the build output will be :)

[bsmith-cycorp]

Anybody still looking into this? The merge appears to have failed, but nothing was done in response

[jstafford]

I managed to get it building locally, and discovered that although my proposed modifications to the patch from @koddsson might have been good ideas, they did not resolve the issue completely. A more through change was required, if I recall correctly. I ended up just extracting the bits I needed from his patch and dropping them into my flow-typed folder. I've posted it as a gist here https://gist.github.com/jstafford/97de1d8e20483f08658dbaad95dec780

[nmote]

It looks like this exposed some performance problems with Flow. I'll take another look and see if those perf problems still exist, and see what needs to be done from there.

[FezVrasta]

Is there any chance to have this merged after the recent performance improvements introduced in Flow?

[nmote]

@FezVrasta, what performance improvement are you referring to?

[FezVrasta]

I'm referring to the improvements mentioned in this blog post: https://medium.com/flow-type/what-the-flow-team-has-been-up-to-54239c62004f

[nmote]

Those are architectural improvements, and unlikely to help with specific pathological performance issues. I'll take another look at this sometime this week, but I'm not optimistic.

[goodmind]

@nmote what performance problems?

[ArcaneEngineer]

@nmote Why is this merge blocked? This has been outstanding for ages.

[nmote]

Earlier in this thread, @gabelevi described the performance problems that this used to lead to. My recollection is that they were still there when I took another look, and I have no reason to believe they have been resolved. It's blocked because we don't want Flow to take 30 seconds to start up on an empty project, and it hasn't been closed because this is still something we would merge if these issues are resolved.

If somebody wanted to champion this, they could rebase it and check whether the performance problems are still present. If they are, they could try to figure out what part of the libdefs are causing the pathological performance problems by e.g. removing parts of the libdef until it's fast again.

Then, they could figure out how to work around the performance issues and/or file an issue with a small repro at which point it's possible that someone on the team would take an interest in addressing them.

Feel free to cc me on any resulting PRs or issues, or drop into our Discord if you'd like to discuss in a more synchronous setting.