svgdotjs / svg.js

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

fix: add Fragment declaration in svg.js.d.ts #1309

Closed we125182 closed 10 months ago

we125182 commented 10 months ago

this pr is to fix issue #1307 .

Fuzzyma commented 10 months ago

Would it make sense to let fragment simply extend from Container to avoid duplication?

we125182 commented 10 months ago

Actually, I thought in this way. After read source code, find that Container extends Element, but Fragment extends Dom. if Fragment extends Container, it'll has Element methods, but Fragment doesn't have methods of Element

Fuzzyma commented 10 months ago

mh, maybe you can add an interface "Containerable" which has all the methods and both fragment and container extend from it

we125182 commented 10 months ago

I tried the Interface way. But implements Containerable Interface, you need define same medthods too. in this way, need define same method three times(Containerable, Fragment, Container). Certanlly use interface can ensure Container and 'Frament` has same Constuct Element methods.

image

try on TS PLAYGROUND

Fuzzyma commented 10 months ago

Alright, last straw: use a class instead of an interface as intermediate layer. But don't export that class since it doesn't exist

we125182 commented 10 months ago

I use the dynamic extends to avoid dupliacted declarations. But the temp varibles(ContainableDom, ContainableElement) always export. which means user can import them, actually they didn't exist. I add comment to explain. Maybe there has a good way.

Fuzzyma commented 10 months ago

I don't think anyone will ever import them. If everything else works like intended I would consider this a good solution :). Does everything work?

we125182 commented 10 months ago

Yes, I linked the changed package in a project. Fragment has methods of Containable and Dom. Container keep same behavior.

image
Fuzzyma commented 10 months ago

Awesome. Thank you!

we125182 commented 10 months ago

Glad to help.