Force users to use FabricObject instead of Object、FabricImage instead of Image、FabricText instead of Text #9172
Conversation
|
we have discussed this. |
fabric.ts
Outdated
| export { FabricObject as Object } from './src/shapes/Object/FabricObject'; | ||
| import { FabricObject } from './src/shapes/Object/FabricObject'; | ||
| /** @deprecated Object is a keyword. Please use FabricObject instead */ | ||
| class Object extends FabricObject { }; |
There was a problem hiding this comment.
Should be const Object = FabricObject otherwise this adds a runtime class level in the hierarchy
There was a problem hiding this comment.
Could you also?
export { FabricObject, FabricObject as Object }
zhe-he
changed the title
|
I suggest we hear from @asturur npm run lint -- --fix
npm run test:jest -- -u
npm test -- -a |
|
I'm fine with the double export as long as it appears as deprecated in IDEs that support JSDOCS, if to have them appear deprecated visually we need to create a new class, go for it. Initially i though people could just do import as, but then after doing it myself a couple of weeks i have seen this coming. Image => Picture ? Text and Image are stronger for me, so i see them better even with the Fabric prefix in front |
src/shapes/Image.ts
Outdated
| classRegistry.setClass(FabricImage); | ||
| classRegistry.setSVGClass(FabricImage); | ||
|
|
||
| /** @deprecated Image is a keyword. Please use FabricImage instead */ |
There was a problem hiding this comment.
i would extend this:
WARNING the old fabric.Image class can't be imported as Image because of a reserved word clash.
For this reason it has been renamed to FabricImage. Please use FabricImage in place of Image.
| classRegistry.setClass(Image); | ||
| classRegistry.setSVGClass(Image); | ||
| classRegistry.setClass(FabricImage); | ||
| classRegistry.setSVGClass(FabricImage); |
There was a problem hiding this comment.
What happens to JSON exports old and new? <--- this is important.
|
There are not many names left for us. If for brevity, can we just add F in front? |
|
I do not like it but we can rename to StaticText like StaticCanvas |
|
if we don't have ideas FabricText and FabricImage are all ok. |
|
We moved away from type being the class name because of minifying, right? |
|
anyway as soon as test passes we are good to go since we don't have groundbreaking names |
|
Gave a stab at fixing tests, i can't run test locally because of M1pro, including the browser, so i did by gut feeling |
|
If you are ok with class names this is fine for me. |
…o v6.0.0-beta12-patch
| * For this reason it has been renamed to FabricImage. | ||
| * Please use `import { FabricImage }` in place of `import { Image as FabricImage }` | ||
| */ | ||
| export class Image extends FabricImage {} |
There was a problem hiding this comment.
I think these 3 should reside in the main fabric.ts file and not in the def files
for me no, i just learned the order of the new files in the tree |
|
ok do not know how to fix jest now |
|
it's OK now for jest. |
|
Leaving this to you guys, you had some open topic on those names. |
|
Is there any other decision to make on this PR? or is good as it is? |
|
@zhe-he @ShaMan123 i don't understand if you still have some internal discussion here or if we are done |
|
Nope |
Object is a keyword. Force users to use FabricObject instead of Object