search

svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.08k forks source link

add foreignObject to types, and get List working #1123

Closed pragdave closed 4 years ago

pragdave commented 4 years ago

I found that I was getting type errors with Lists when it was an interface, but if I changed it to a class and tightened up the callback function and return types it was OK.

I also added the ForeignObject class and helper function in Containers

coveralls commented 4 years ago

Coverage Status

Coverage remained the same at 99.924% when pulling 5bd6906f48ad528cb3d3ee980c5bd537b703d953 on pragdave:some-more-typings into f9055655cec603e2cb7d01c2307a1eafab1b3f2e on svgdotjs:master.

pragdave commented 4 years ago

ElementAlias is not an interface. It is just all svg.js types as a union. You would need an interface with ALL the methods which are available on any element of svg.js.

But if we said:

class ForeignObject extends ... implements Rect {
}

then wouldn't that means that it would be typed to include all the methods in Rect? So wouldn't the union type say 'all the methods in any of these..."?

Fuzzyma commented 4 years ago

I honestly have no idea if you can implement Unions. Also: if you implement them you must actually write them out in the typings and thats what I wanted to avoid.

Note to myself: I need to add SVGForeignObjectElement to the typeMappings in the d.ts file

pragdave commented 4 years ago

So I just spent a couple of hours messing around with trying to get a safe typing for List. I've learned a lot about keyof, indexing, and mixins, but I don't seem to be able to create something as dynamic as we need. It's on my back burner of things to think about while asleep.

In the meantime, this PR has turned into something of a mess. I'd like to get at least some of it merged, just because it fixes a lot of errors I'm seeing here. This might be a case where pragmatism beats perfection :)

Would it be best to close this PR and open another? Or do you know what you're happy to merge?

Fuzzyma commented 4 years ago

So I just spent a couple of hours messing around with trying to get a safe typing for List. I've learned a lot about keyof, indexing, and mixins, but I don't seem to be able to create something as dynamic as we need

Yes, I honestly thought, that a string-index-key as I tried would allow this but you can either have that OR other methods. Because all the array methods have to follow the string-index signature, too. Its stupid. Thats why it cannot really be typed and that is really unfortunate.

In the meantime, this PR has turned into something of a mess. I'd like to get at least some of it merged, just because it fixes a lot of errors I'm seeing here. This might be a case where pragmatism beats perfection :)

I honestly lost track a bit :D. I will have a look which conversation can be resolved and will do a last review of what needs to be changed explicitely. Maybe I just change it myself if that works over the browser. And then we can merge :)