asturur commented 2 years ago

Hello everyone!

FabricJS is nearly 12 years old.

The library has still its place around the internet, is good for many things, is never perfect, but i know that for who approach rich canvas based interactive apps, this is a nice piece of software to leverage.

While our main fault are still lack of great docs and updated and clarifying examples, now also the javascript code looks like a lot obsolete.

There are things we could leverage like Promises, default value for arguments, fat arrow for loops and readibility, native classes, proper getters and setters.

We would like to take the occasion to revisit the shape of the codebase to something more modern. This would include proably a bundler, proper import statements instead of a single file that gets built like lego blocks, maybe tree shaking for who import just what he wants to import.

This would make the custom built unnecessary, using other libraries easier including touch interactions.

So a move to es201X is coming anyway.

The question is if we want to add type descriptors or if we want to make a TS migration right away.

The benefits of consuming TS are clear to me, the benefit of writing typescript a little less, i'm not a great fan and also i m not great a TS. Is very easy to use TS in the wrong way and produce subpar results.

We are open to suggestions and understand what is the best way to make everyone happy. external type definition in form of d.ts files are a good middle way if there is a way to enforce correctness at build time. A new lint profile is required. A new build tool too. Legacy JS support is not going away. A bundler will produce the correct code for older browsers. But probably IE11 is out of the game anyway. I would love to have 2 builds, a legacy one and a modern one ( the classic last 2 versions of each browser )

So which are your reason to list TS on the pro/cons of this change? I understand you may not like TS. that is fine. I don't like it either. But if is terse, with types out of the way, and a transpiled build is available, what is your main point against or for it?

yassilah commented 2 years ago

Hi! I don't really understand the use of a tool to generate types tbh? I think the best way to go is migrate files from .js to .ts going from least dependency to most dependency (e.g. go from utils > mixins > shapes > collections > canvas or something like that). For each file, there may be a need for special interfaces to avoid repeating args, which we could declare in a /types root folder to allow access from other files. As for tools, we could think of using ts-mixer, which makes it quite easy to reuse mixins across classes. We could also include a small mixin utility to avoid any external dependency.

ShaMan123 commented 2 years ago

Because it a big effort and many of us want to help we need to think and discuss how to break down the work so that everyone can take part in a good and useful way. We will need to make some kind of table assigning what to whom and to keep track of progress, maybe even use some teams tool. We will definitely need to maintain a commons PR that will be updated frequently and everyone will have to update their branch all the time to stay in sync. And add some tools and how tos to make life easier, plus PR guideline and reviewing guidelines and tracker issues. We must address all open PRs as well so that they won't die out in light of this major change. I think we need a bit more time for preparations, to be honest. This change is a chance to establish a plugin concept. Classes will be rewritten so we should take advantage of it. I believe renaming will be done as well, as part of deciding what's private/public. Too many useful methods start with _. And changing of awkward function signatures. Anyways, if you are working on something state it clearly ASAP so we can discuss it and to avoid unnecessary work. And perhaps target, as suggested, the independent modules first. POCs should precede major changes (that's for the guidelines 😉). It might be a good idea for us all to target utils first, complete it and move on. This will require fabric to be in a transient mode which is possible I guess but a build tool will need to be setup in advance to support this. Exciting times!

ShaMan123 commented 2 years ago

BTW if any of you want to help in preparations there are some simple PRs that can be reviewed. It's a good way to understand how to use the dev tools. We can help.

kirill-konshin commented 2 years ago

FYI https://devblogs.microsoft.com/typescript/a-proposal-for-type-syntax-in-javascript/

Btw, you can migrate to TS by allowing plain JS and add compilation step to build. Once this is done you can start to transition types from JSDOC to TS file by file at any pace.

Lazauya commented 2 years ago

Hi! I don't really understand the use of a tool to generate types tbh? I think the best way to go is migrate files from .js to .ts going from least dependency to most dependency (e.g. go from utils > mixins > shapes > collections > canvas or something like that). For each file, there may be a need for special interfaces to avoid repeating args, which we could declare in a /types root folder to allow access from other files. As for tools, we could think of using ts-mixer, which makes it quite easy to reuse mixins across classes. We could also include a small mixin utility to avoid any external dependency.

I've been doing research on existing tools, and it doesn't seem like there's anything that would fit this project's needs, so I'm thinking that this will end up being the best approach.

BTW if any of you want to help in preparations there are some simple PRs that can be reviewed. It's a good way to understand how to use the dev tools. We can help.

I can start with this for sure. Any specific PR's?

ShaMan123 commented 2 years ago

I can start with this for sure. Any specific PR's?

7714 #7713 #7689 #7805

7719

7775 #7776

7806 #7802

Lazauya commented 2 years ago

I can start with this for sure. Any specific PR's?

7714 #7713 #7689 #7805 #7719 #7775 #7776 #7806 #7802

For newcomers like myself, what is the review process? Is it codified anywhere? Are these just simple code reviews?

ShaMan123 commented 2 years ago

First read the CONTRIBUTING Guide

code review
clone PR
run tests and check visual diffs by running npm test -- -a -d
start fabricjs.com (npm start) and test the changes live

~~I am working on better dev tools as well to make this process easier.~~ DONE #7825 #7833

pollow commented 2 years ago

Not a hardcore JS expert but has been working on JS on and off for many years. I think with ~EMCAScript 2020, the latest JS, TS has been playing less important roles. For many times you can see, TS compiled results are identical to vanilla JS. And backward compatibility can be done by babel polypill. But since this is a new version with new standards, do we really need IE8- support?

As a 3P library, I would vote for less dependencies on tools & languages, but using vanilla JS to be closer to standard.

But type system is definitely a bit plus for any larger project, especially with more collaborations. FlowJS might be a good approaches, which is slightly more closer to the new JS optional type annotation proposal. And the type can be stripped off easily, be added progressively., and generate .d.ts by other tools in babel when release.

ShaMan123 commented 2 years ago

I guess this should be discussed here...

As part of the migration I want to get rid of these:

[ ] extend/clone in favor of clone | cloneDeep | assign by lodash - currently it doesn't support circular refs..., lodash does and is way better
[ ] left/top in favor of x/y - this will be very easy once everything is written in TS. VSCode does it in a heartbeat.
[ ] shared object controls. There's no advantage in it, only disadvantages. IMO it MUST GO! Look at the loading times. Comparing against fabricjs.com shows there is NO real diff. #7914

asturur commented 2 years ago

we can probalby stop using extended/clone with standard es6 tools. I don't want to have lodash as a runtime dependency. We should not need to clone stuff at all that often. Most of the options are clonable with { ...options }. We clone to defend against mutation, all the other situations should be avoided.

left/top vs x/y: x/y makes more sense, yes. Seems a big headache tho.

All the disavantages of shared controls is the textbox edgecase confusion, but you can prove me wrong.

svjansen commented 2 years ago

I've to say we're using fabric.js in a very large project with ES6+ and esm of course. We had to rewrite a few little things and even got some extending classes here and there but dont have any problems to migrate everything we need.

However I can't see any reason why you should use TS in a rewrite. We're working with JetBrain tools and JSDoc and never had any kind of type problems. If we find something we need to "param guessing" we write a little wrapper class that got the exact same function names and just build JSDoc around.

xorb commented 2 years ago

Definitely go with typescript. Why not ? Type system is a great plus for any larger apps plus we can create a build that everyone can use it (js users, ts users, etc).

asturur commented 2 years ago

https://github.com/fabricjs/fabric.js/pull/8019

This is a small pr that change the build system to add TS and ROLLUP + TERSER.

If you have suggestions on the configuration is welcome. The idea is that we support a certain sets of features, that allow most browsers to work. In my example i picked up chrome 58 because is from around 5 years ago, it supports the spread operator natively. I m not sure there is much advantage of restricting more and more, or to then transpile with babel to IE11+.

In order to do so i left typescript with es2022 target trying to have ts compiler not touch anything related to code itself. Compilation is slow! 12 seconds.

asturur commented 2 years ago

Definitely go with typescript. Why not ? Type system is a great plus for any larger apps plus we can create a build that everyone can use it (js users, ts users, etc).

Is a matter of preferences. I can go with my super personal list of i don't like typescript because...

is from a company well known to shut down projects when they are no more profitable
is a language that does not run natively in the browser
it does what JSDOC alrady is doing for me
sometimes is silly like today i had angle = -angle and no type wasn't matching
typechecking erros with complex types are unreadable for me

I add it because the comunity likes it and i m always a pass of transpiler away to go back to JS if the shit hit the fan.

I don't like it. Rationally speaking isn't bad, and is a plus for developers and learners, and so that is why it gets done.

asturur commented 2 years ago

oh the current index.js to build is transitory of course. It is because we have this mixed situation in which we need to move the files to TS and i didn't want to have a PR that was renaming and changing all files and the build system.

When we are done, the import from and export from should do the job ok. Then we have to figure out which is the best setup for custom builds. the npm package should have the dist and the rest, so you can do something like import { Canvas, Text } from 'fabric' And maybe not having to import all fabric inside the app if you are doing just some text rendering.

svjansen commented 2 years ago

@asturur maybe you should keep in mind/should know, that Microsoft got a proposal on TC39 currently in Stage 1. In short: they want to add static optional typing to JS in TypeScript format that should be ignored in runtime.

MS DevBlog tc39 GitHub tc39 official page

asturur commented 2 years ago

the proposal is actually typescript when i stop thinking. The types never go to runtime, because no one wants to download or parse them at runtime, no one event wants the added code in the parsers to ignore them. That is why JSDOCS is so good, it is typescript in comments, with descriptions and link to examples and other things.

Why they didn't write a type checker for jsdocs?

i m not a runtime expert, we all want performances for js, does the extra pass of parsing + stripping cost me something? Does it make sense if then any minifier will strip them away for good? I get they want to avoid sytnax conflicts with future ecmascript features so that the TS ecosystem doesn't get stuck with incompatibilities with es20XX.

Edit: maybe i prefer that syntax space used for features 🤷 i can use at runtime.

svjansen commented 2 years ago

The only thing they say about JSdoc is this. What they talk about over and over is that ppl trying to get rid of unnecessary build steps.

I can say that this would make things easier if you got multiple objects with same name where you would need to use namespace/modules syntax on JSDoc. A Problem I have in JSDoc are types not instantiated in a js file itself. Lets say I got factory for an fabric.Image in a class named ImageHandler, but the ImageHandler itself never instantiate a fabric.Image. In this case it feels like luck if JSDoc and the IDE get the correct Object. In the proposal you would import fabric.Image just for typing to avoid such a Problem.

jimmywarting commented 2 years ago

I'm somewhat embarrassed by the optional Type Annotations proposal, it will literally not do anything useful for the web parser/runner. it will only increase the complexity of the browsers parser and making it slower. you will not get any error if you pass in a wrong type, it's only a loosed type "annotations" made for type hinting/autocompletion. it's just for ppl who are lazy to compile their code into js and simply just wish to run typescript files as it's written. 👎

Optional Static Typing on the other hand promise to deliver things such as true types support into the languages such as floats, ints. It's something that change the underlying language and how it works. And it's something that also probably something closer that could be compiled to wasm if you wish (unlike typescript). IMO this is much better... it's way better if you can decleare a small int variable to only take up 1 byte instead of creating a variable that is by default of type float in javascript when you are creating numbers. and they can't be changed to something else 👍

ShaMan123 commented 2 years ago

8020 exposes a mechanism that is in charge of transforming `src` files into es6 syntax (and convert to `ts` files).

Thanks to VSCode, adding types to typescript files based on JSDOC (=transformed files) is automated and extremely simple so we have that to our advantage which is GREAT (see the PR description for more info). Meaning that the combination of the transformer and VSCode is very powerful.

It's going to save lots of time and energy and allow us to focus on the essence of the migration.

People who want to help with the migration can really focus on types and stuff instead of syntax and replacing commas. On top of that, this will allow us to migrate and keep existing PRs (at least most of them I hope) which was a big concern of ours.

Take a look at a fantastic example of types by @asturur here

bhabgs commented 2 years ago

I support using typescript and hope to support the Chinese market better. Because I'm from China....

ptgamr commented 2 years ago

First of all, thanks for the great work you all put in so far to maintain this great library. I've been a happy user since a year or too with "somewhat" usable types from @types/fabric (still at 4.5, while fabric already 5.x)

Please rewrite in TypeScript, it'll make the code more accessible to developers nowadays and I believe will lead to even more adoptions. I myself also not a fan of TypeScript around the time of 2018, but after being forced to use it, I would say I can't really go back now - the DX is so great and I can focus more on developing features with more confident that I'm not breaking stuff (consider types as a first level of defense, or another kind of unit test). Being a library, it's even more important to not breaking stuff.

It's really frustrating when one working on a TypeScript project (which I believe A LOT of people do these days), and the library we want to pull in doesn't have good type defs. Usually, the one with good typings will win (either written in TS, or having types included in the repo).

Konva did the migration in 2019, and see the NPM chart, they already took over fabric.js at some point: https://github.com/konvajs/konva/commit/4d58cd647912d0fd3cd7b9ce619a6858347e1464

2022-07-06-014700_1319x672_scrot

I'm pretty sure it's correlated to the rate of TypeScript adoption, which I myself is a part of that problem :-)

ShaMan123 commented 2 years ago

Hello community,

TS is coming 🤩

A Quick Update

The old build system has been replaced with rollup #8013 Now we focus on migrating to TS. The team, @asturur @melchiar and I @ShaMan123, decided for! It is a done fact. The first milestone would be moving to es6 not caring about types and focusing on import/export. After that it's a type festival. The es6 step is a bit tricky so the team is doing that (contributions are still welcome, contact/ping us if you want in). The second step is the main event for TS fans, the type-fest 🥳. Everyone is welcome aboard. We will decide how to manage that later on.

Why I am writing this update

We are reaching out to the community after a POC. We have tested migrating to classes (which is granted) but we encountered 2 hurdles that must be mitigated. Or else class will be disregarded as unwanted 😥.

We have considered some approaches and would like to hear your ideas and considerations.

Guidelines for the solution

code must be simple and easy
NO duplicate values/variables
NO ugly assignments
support a way to subclass/override these values with ease at runtime (this part is a bit flexible in terms of when and how the overriding is done)
untranspiled/transpiled code MUST behave similarly
TS compatible - type inference should work out of the box

(1) Default values

fabric uses the default values defined on the prototype of a shape (Object, Rect etc.) to remove default values when exporting to json, see here. The problem is that TS class properties are not set on the prototype. They are assigned to the instance in the constructor during initialization, build #8101 to see for yourself.

So there isn't a way to get default values from the class.

Thoughts

assign defaults to prototype in class static init (failing 2, 3, 4)
export a static method getDefaultValues (failing 2, 3 if we are honest)
deprecate removeDefaultValues and change toObject(propsToInclude) to populate ONLY propsToInclude, ignoring the rest (I prefer this approach)

Tryouts

generating files with default values during build #8105

(2) Subclassing a superclass

Let's say you are a dev using fabric@v9 a year from now. Everything is there, typescript, classes (maybe even webgl, who knows?!). You need to override a method defined on fabric.Object but you want the change to propagate to all classes.

How do we support that?

Thoughts

We could convert all subclasses to mixins

export function RectMixinGenerator<T extends new (...args: unknown[]) => unknown>(Klass: T) {
    return class RectMixin extends Klass {
        //   rect class body
    }
}

And then you could create custom objects by using the mixin with your subclass of Object


class ObjectOverride extends fabric.Object {
    someMethod() {
       // override logic
    }
}

export const Rect = RectMixinGenerator(ObjectOverride);

But it seems TS is finding it hard to digest this approach #8100 (see applyMixins)

avra-m3 commented 2 years ago

Hey @ShaMan123 , Happy to lend a hand on smaller tasks outside of work hours, I'll send you an email.

The typescript migration for a library like fabric is never going to be perfect, I think class mixins will be fine but will likely leave a lot of existing applications on fabric 5, there should be a plan to support those applications with security updates, etc in the medium term if that's the path we go down. Regarding the first point, given the migration is already going to be a nightmare I believe you should aim for the best solution rather than the one that makes the migration the simplest. In our use case we've never used removeDefaultValues and leaned heavily on toObject(propsToInclude).

Look forward to seeing the results!

Lazauya commented 2 years ago

@ShaMan123 I'm having trouble understanding the requirements about overriding during runtime, specifically the requirement "support a way to subclass/override these values with ease at runtime (this part is a bit flexible in terms of when and how the overriding is done)"

Does this mean that for (1) you should be able to set the default values during run time? Like I should be able to set the default border radius of a rectangle, and then that becomes true for all rectangles?

For (2), does this mean that you wish to be able to override a method like getTotalAngle() and have all rectangles use your new method?

(1) might be achievable using TS decorators inspired by this Stackoverflow post. This is a more a complete example. My understanding is that if you want to override stuff at runtime, you'd just need to modify the prototype.

ShaMan123 commented 2 years ago

@Lazauya yes to both of your questions. The decorators approach is interesting (but is still experimental, we must take that into consideration). In total is viable and good and meets all guidelines.

My understanding is that if you want to override stuff at runtime, you'd just need to modify the prototype.

TS Playground Link shows you are right regarding modifying the prototype but in such a case modifying a prototype doesn't pass down to subclasses due to the strict nature of class.

ShaMan123 commented 2 years ago

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier

~~I wonder what happened after v3.7~~ EDITED https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-3.html#usedefineforclassfields-now-defaults-to-true-on-esnext-and-eventually-on-es2022

@asturur

Lazauya commented 2 years ago

@ShaMan123

modifying a prototype doesn't pass down to subclasses due to the strict nature of class.

Actually, I think this behavior is favorable. I didn't even know this until just now, but it actually DOES override in the subclasses if you haven't overridden that function explicitly in the class definition, which I think makes sense. See this example.

I mean, I guess I just don't know a use case where you would have a base class like (lets call it Base) and a derived class (called X) where you have a different definition for some function f in Base and X. It doesn't make sense to me that changing the prototype of Base would change X.f too if X has a completely different definition of f. It make more sense that you would simply change the the prototype of X directly if you wanted to make them both the same.

ShaMan123 commented 2 years ago

@Lazauya I agree with all you wrote. Excellent, thanks. @asturur take a look

asturur commented 2 years ago

The only thing i want to investigate so i get a grasp of it is why it behavea differently. Mdn says, class definition is just a syntactic sugar on top of functions. FabricJs probably with createClass was doing some extra magic that i m happy to remove as soon as i understand it fully. If functions do not support prototype override at runtime i l fine with that

Meesayen commented 2 years ago

Hi, I was thinking of a possible solution to (1) Default Values, and thought we could maybe quickly create a dummy instance of the same class as the object, during the _removeDefaultValues function execution and then compare the two, e.g.:

   // from this
    _removeDefaultValues: function(object) {
      var prototype = fabric.util.getKlass(object.type).prototype;
      ...
    }

    // to this
    _removeDefaultValues: function(object) {
      var prototype = Reflect.construct(object.constructor, [])
      ...
    }

Now the only problem I see is that some classes may have mandatory constructor arguments, and in that case that Reflect.construct line would throw unless we pass the necessary arguments list :/ For Fabric classes might be an easy problem to fix, AFAIK most of the classes either have an optional options object, or an element + and optional options, but for custom classes in the wild, maybe we'll need to ask users to provide a getDefaultConstructorParams which I guess breaks guideline 3.

ShaMan123 commented 2 years ago

At this point I think we should drop removeDefaultValues. The dev can easily override classes once #8203 in done. As mentioned in previous comments, proto assignments are possible (without inheritance). It will be breaking and migrating will be an effort. But it will be worth it.

jiayihu commented 2 years ago

From the consumer POV, one of the biggest headaches with using fabric in our company has the heavy reliance on custom behaviours in the class implementation. Just as an example, I've found sometimes that using callSuper("originalMethod") in an overridden method results in unexpected results compared with originalMethod.call(this) because of how callSuper actually works.

So as @Lazauya , I'm in favour of following the strict standard, even if it can result in more boilerplate code or extra work, as far as it keeps the behaviour consistent with the standard.

Regarding the transition to TS, it's late for a feedback, but it would be very welcome from us. I've actually often seen minor errors or inconsistencies in the fabric codebase (when overriding) that would have been avoided if TS were used internally as well. Actually, sometimes errors in the manual d.ts files have required us extra work to workaround it, such as aliasing a method to a different name so that we can give it the correct type. We even considered cloning the repo and including it into our TS codebase due to the high customisations we made over these years and the benefits of TS for us.

Lazauya commented 2 years ago

@asturur

The only thing i want to investigate so i get a grasp of it is why it behavea differently.

I made a TS Playground here to better understand what extra stuff fabric was doing here

In short, createClass really isn't doing much. It's literally just a more verbose and less safe version of the normal ES6 classes. It handles mixins by progressively merging all the objects passed into it. The only thing notable is how it handles it handles super methods. If you use callSuper somewhere in a method you define, it will wrap the method in another method that sets the superclass property so that callSuper can work recursively in super methods, and won't get confused. It checks for callSuper by looking at the text of the function, and I feel like you can confuse it somehow; ES6 super doesn't have that problem.

As far as I can tell ES6 classes should literally just be drop-in-able if you replace all the calls to callSuper. It needs a new mixin system too but I think @ShaMan123 did that already.

ptgamr commented 2 years ago

@Lazauya @ShaMan123

(1) you should be able to set the default values during run time? Like I should be able to set the default border radius of a rectangle, and then that becomes true for all rectangles?

Just want to chime in here, in our application this behaviour creating issues as we have two different canvases behave quite differently in the same view - setting the default values in one canvas will have unexpected consequences in the other canvas and vice versa.

And I think I don't mind creating my own classes to provide my own default values, ie.

class MyRectWithSomeDefaults extends fabrics.Rect {
  constructor() {
    // set the default values in here
  }
}

atmikev commented 2 years ago

I'm relatively new to fabricJS and am glad to see its getting an update. I'm a little confused on why you would want default values for base objects though. @ptgamr is right that subclassing would be the right way to do that.

asturur commented 2 years ago

https://github.com/fabricjs/fabric.js/pull/8246

Expanded my concerns a bit here in this PR with some example code ( this is a discussion pr is not being merged ro anything like that ).

Yes subclassing is a good way to do it, you can do it, this is not stopping subclassing at all. But also:

subclassing is not for everyone and a lot of people got to the same end result changing the values on the prototype
subclassing Text and having it stay in the middle of the prototype chain between Object and IText or Textbox is even harder
You don't have to do prototype changes if you don't feel like, it is just an alternative less verbose approach

asturur commented 2 years ago

If i want to add an extra property to the cacheProperties, or remove one, if i want to change originX/originY for my project to center/center, or if i want to work with objects with default stroke to 0, and if i want to assign a different control set to a class, i don't want to force a developer into subclassing. It was never necessary, it shouldn't be necessary now.

On top of the issue with default values that yes, can be solved writing other code, but again, why?

I was missing the knowlege that the class fields aren't the old properties you would add to the function prototype, are a different thing specced in a different way. If i knew that i wouldn't even started the topic.

class Rect {
  type: 'Rect'
}

is not supposed to be syntactic sugar over

class Rect {
}

Rect.prototype.type = 'Rect'

But is indeed syntactic sugar for

class Rect {
  constructor() {
    this.type = 'Rect';
  }
}

That is a different thing that today fabricJS does only for the options you pass in the constructor, and you will be still able to do that.

asturur commented 2 years ago

@Lazauya @ShaMan123

(1) you should be able to set the default values during run time? Like I should be able to set the default border radius of a rectangle, and then that becomes true for all rectangles?

Just want to chime in here, in our application this behaviour creating issues as we have two different canvases behave quite differently in the same view - setting the default values in one canvas will have unexpected consequences in the other canvas and vice versa.

And I think I don't mind creating my own classes to provide my own default values, ie.
class MyRectWithSomeDefaults extends fabrics.Rect {
  constructor() {
    // set the default values in here
  }
}

In particular which default values where you setting that were bleeding over the other canvas?

asturur commented 2 years ago

The only thing notable is how it handles it handles super methods. If you use callSuper somewhere in a method you define, it will wrap the method in another method that sets the superclass property so that callSuper can work recursively in super methods, and won't get confused. It checks for callSuper by looking at the text of the function, and I feel like you can confuse it somehow; ES6 super doesn't have that problem.

As far as I can tell ES6 classes should literally just be drop-in-able if you replace all the calls to callSuper. It needs a new mixin system too but I think @ShaMan123 did that already.

Yes es6 classes are drop-in repleaceable, and the mixins can be added as before, assigning them over the prototype, no changes are necessary, and the resulting class is perfectly valid

ShaMan123 commented 2 years ago

I prefer chaining mixins with a mixin builder (e.g. Collection) as mentioned in mdn. That way you can subclass Text and recreate IText with your subclass. Sure it requires a few more lines of code. But it releases fabric from a patched and confusing js standard. That is the whole point of the migration. So to me it seems fit that we become strict in complying to it and drop what doesn't fit anymore.

asturur commented 2 years ago

chainging mixins is doable, it changes nothing, but what you see as confusing and patched ( patched why? the JS defines the prototype inheritance in a certain way, and classes leverages it entirely ) may not be for other developers. Extending a class vertically and/or horizontally is just a matter of what fit best with the model.

Rect is a superset of Object, while for example SVGexport is a functionality you can add to any of the classes or not, without changing what the class is. a Rect stays a Rect, without the need to become a SVGEnanchedClass extends BaseClass.

Is also possible that this can be done at lower level doing:

Rect.__proto__ = SvgExtender(Rect.__proto__)

so adding it programmatically to already existing classes, but for the scope of how fabricJS code is written vs how is used, nothings change.

The fact that when i open the VSCode editor i want to see the code organized in a certain way that conform with my liking is something i had to put away when i became the not only maintainer anymore.

So i try to stick to measurable changes, and everyone should do the same.

how much code
which functionalities
easy of use
enabling new patterns vs breaking the old one
eventually speed and correctness

So i prefer, what i prefer? i prefer the old functions. Are perfect, i don't even like all the fuckery of brackets i have to write for TS. <N, T> T extends N | never | notNullable but those are adding functionalities and possibilities and are not removing any ( well some... ) so i move on and i accept them. Classes are not blocking anything if i don't jump on the public fields, and probably have a more performant super, faster overall execution because the code is statically defined and not dynamically added (reading MDN ). so good to go for me.

ShaMan123 commented 2 years ago

Could you define the next steps as you see it? I would prefer seeing a PR you've done to understand the new standard. Or make a list of dos/donts

asturur commented 2 years ago

i can convert object for the sake of seeing how it would look like. The next step would be pull out what we lose if we don't add public fields in the fabricJS codebase and evaluate. From my small deep dive, we don't lose anything. Which are the different opinions?

ShaMan123 commented 2 years ago

I suggest leaving Object to the end if possible. It will be complex and messy with all the mixins and logic broken all across fabric, some even shared with canvas. I would choose Rect or Circle, the simplest possible, wondering if it will be a viable POC if Object remains as is (??).

I am not fully grasping your point, forgive me. I will need to see code and review and ask and discuss then. Sounds to me that some of these fields (e.g. cacheProperties) should become static. Defaults of course can't.

asturur commented 2 years ago

Well but is exactly with the mixins and complexity that you can judge if an approach is good or bad, Circle will look good whatever you do, is very few lines. I don't have to do a full functional object all in one go, but a PR that shows the code will be clear enough. On top of the thing that i suppose we have to let go the name Object, and use something else, like BaseObject? fabricObject?

ShaMan123 commented 2 years ago

sounds good naming should be changed for sure Shape?

asturur commented 2 years ago

Shape is too similar to what then people call generically all simple rects, circle, stars, and all the geometric basic figures. Also the Object comes with no shape, since has no render method. It is just our base class and we can't take the reserved word Object, BaseObject or BaseClass or FabricObject something like that. _Object is too lame

Lazauya commented 2 years ago

I vote FabricObject. I think it's the most clear and will be the least confusing when used in other codebases.

fabricjs / fabric.js

IMPORTANT: Migrating to modern javascript/typescript #7596

7714 #7713 #7689 #7805

7719

7775 #7776

7806 #7802

7714 #7713 #7689 #7805 #7719 #7775 #7776 #7806 #7802

8020 exposes a mechanism that is in charge of transforming `src` files into es6 syntax (and convert to `ts` files).

Hello community,

A Quick Update

Why I am writing this update

Guidelines for the solution

(1) Default values

Thoughts

Tryouts

(2) Subclassing a superclass

Thoughts

fabricjs / fabric.js

IMPORTANT: Migrating to modern javascript/typescript #7596

7714 #7713 #7689 #7805

7719

7775 #7776

7806 #7802

7714 #7713 #7689 #7805 #7719 #7775 #7776 #7806 #7802

8020 exposes a mechanism that is in charge of transforming src files into es6 syntax (and convert to ts files).

Hello community,

A Quick Update

Why I am writing this update

Guidelines for the solution

(1) Default values

Thoughts

Tryouts

(2) Subclassing a superclass

Thoughts

8020 exposes a mechanism that is in charge of transforming `src` files into es6 syntax (and convert to `ts` files).