Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Will Fabric v6 support user-defined Object properties? #9845

Closed
4 tasks done
wangrongding opened this issue May 3, 2024 · 15 comments
Closed
4 tasks done

Comments

@wangrongding
Copy link

CheckList

  • I agree to follow this project's Code of Conduct
  • I have read and followed the Contributing Guide
  • I have searched and referenced existing issues, feature requests and discussions
  • I am filing a FEATURE request.

Description

Will Fabric v6 support user-defined Object properties?

For example, when I want to create an element, I can embed an Id in the object for convenient future use. Currently, Fabric v5 does not support this due to ts type restrictions, so I am currently using Object.assign to merge the ID with the target object.

Current State

image

Additional Context

No response

@asturur
Copy link
Member

asturur commented May 3, 2024

fabric v6 sort of supports that but the definition of types are hard to do.
This should definitely be explained in some doc.

@ouyoungs
Copy link

ouyoungs commented May 7, 2024

mean this?
https://juejin.cn/post/7153068432303325220

let canvas = new Canvas('c')

let rect = new Rect({
  width: 30,
  height: 30,
  top: 10,
  left: 10,
  my_id: 'rect01'
})

canvas.add(rect)

function handleToJSON() {
  console.log(canvas.toJSON(['my_id']))
}

@wangrongding
Copy link
Author

I know this, I'm talking about the official api and support for Typescript type.

@asturur
Copy link
Member

asturur commented May 8, 2024

i think today you can do:

const newText = new FabricText<{ id: string } & Partial<TextProps>>('myText', {
  id: 'hello',
  fontFamily: 'Arial',
});

But i m sure there will be some kind of hiccups here and there, is not as smooth as we want, is also hard to do because of mixins and everything.

Slowly mixin will go away and hopefully will be simple

@vadimcoder
Copy link

I ended up using this so far (I'm using v 6.0.0-rc2):

interface ICustomData {
  entry: ...; // my custom prop
  isDirty: ...;  // my custom prop
}

// custom PathProps
interface IMyPathProps extends TOptions<PathProps> {
  customData: ICustomData;
}

// custom Path with custom data based on fabric Path
export class MyPath extends Path<IMyPathProps> {
  customData!: ICustomData;
}

// using:
fabricCanvas.add(
  new MyPath("M 0,0 ..." , {
    fill: 'transparent',
    customData: { // TS does type checking here
      entry: ...
      isDirty: ...,
    },
  }),
);

// or:
const path = fabricCanvas.getActiveObject() as MyPath;
console.log(path?.customData);

@zhe-he
Copy link
Contributor

zhe-he commented Jul 10, 2024

Let's solve it by rewriting...

/** fabric.d.ts */
import "fabric";
declare module "fabric" {
    interface FabricObjectProps {
        /** mark 1 */
        someAttr?: string | number;
    }
    interface FabricObject {
         /** mark 2 */
        someAttr?: string | number;
    }
}
/** patch.ts */
const lastToObject = FabricObject.prototype.toObject;
FabricObject.prototype.toObject = function (arr: string[] = []) {
     return lastToObject.call(this, arr.concat(["someAttr"]));
}
image image

@asturur
Copy link
Member

asturur commented Aug 16, 2024

#10071
Made the export of plain custom properties as simple as possible.
Deep objects won't be cloned.
I m also going to cover this topic on a specif doc on the website

@asturur asturur closed this as completed Aug 16, 2024
@asturur
Copy link
Member

asturur commented Aug 17, 2024

@Smrtnyk
Copy link

Smrtnyk commented Aug 17, 2024

I am having issues with properly typing classes that are extending from Group.
For example here https://github.com/Smrtnyk/Firetable/blob/master/packages/floor-creator/src/elements/Table.ts#L104
Group is not a generic so I cannot pass the unique table props to it

@asturur
Copy link
Member

asturur commented Aug 17, 2024

I forgot about that mixin there. I think we should make it Generic

@asturur
Copy link
Member

asturur commented Aug 17, 2024

I played a bit with it and while Props and SProps can still be made generic, i don't think eventSpec can.

@Smrtnyk
Copy link

Smrtnyk commented Aug 18, 2024

I guess at least passing Props and SProps is better than nothing
it would solve my issue at least to not have ugly ts-expect-error

@asturur
Copy link
Member

asturur commented Aug 24, 2024

@Smrtnyk check if some of this code helps: #10092 i m still working on it didn't do deep testing

@Smrtnyk
Copy link

Smrtnyk commented Aug 29, 2024

hm, it is still not really working
there is an error for Props not being assignable to Group props
because Props from documentation refer to serialized props of the group, which includes serialized layout manager, but props in group accept only layout manager.
I guess easiest for you is to add a test in fabric that is extending the group and make sure that there are no ts errors.
if you need test inspiration you can have a look at the link I posted above
https://github.com/Smrtnyk/Firetable/blob/master/packages/floor-creator/src/elements/Table.ts#L104
in my case I am extending the table class on a second level as well
https://github.com/Smrtnyk/Firetable/blob/master/packages/floor-creator/src/elements/RectTable.ts#L24
where I had to add another ts-expect-error.
I assume that fromObject also needs to be more flexible, not just toObject

@asturur
Copy link
Member

asturur commented Aug 29, 2024

Yes is not fixable as it is today.
I really spent 10+ hours on this and the correct way is to drop the collection mixin and duplicate the collection code for canvas and group.
But is like extra 5KB or more of code added to the library and makes me mad.

I'll probably try to resort to the older method in which the methods were copied over the class at runtime and i will duplicate the declarations only. But is time consuming.

For now the best thing you can do is just use the extend on the actual group and use declare and the constructor to add all of your props.

I checked your examples, where is that you are having issues with those? What is not working?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants