-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
TS(): Testing fabric 6 package configuration and code splitting. Fix fabric object access #8622
Conversation
Build Stats
|
rollup.config.mjs
Outdated
name: 'fabric', | ||
format: 'es', | ||
sourcemap: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change makes it work with the 3 setup i tested so far, but is all rollup based, i want to check webpack too and and the standard next.js and SWC setup.
I m not saying i think we should have 2 builds ( or 3 ) but right now i m collecting data on how to ship something that works out of the box and is not tied to a gigantic fabric monolith.
I made some experiments do not take the diffs as definitives yet, looking at options |
chrome >= 58 | ||
chrome >= 64 | ||
safari >= 11 | ||
firefox >= 58 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those for examples are browsers released Jan 2018.
But to be honest i would like to decide by canvas features.
Like which is the minimum set of browsers with advanced text metrics?
For the beta is not relevant we can readjust
} | ||
] | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be an alternative config file, very loose, that would behave as tsc, just stripping types.
This could be useful for developers that want to import files directly and then want to bundle and transpile on their own.
Ok this seems a decent approach, apart i need to fix that we can't add random things to the fabric object, because now it is an export and you can't add things to it, is read only. |
Ok after a bit of pain i m here: This works, isn't exactly what we want now but is a path forward. import { useEffect } from 'react'
import { Canvas } from 'fabric/dist/src/canvas/Canvas';
import { Textbox } from 'fabric/dist/src/shapes/Textbox';
import './App.css'
function App() {
useEffect(() => {
const c = document.getElementById('c');
if (c!.hasAttribute('data-fabric')) {
return;
}
const fabricCanvas = new Canvas('c');
const text = new Textbox('Hello 6.0', {});
fabricCanvas.add(text);
}, []);
return (
<div className="App">
<canvas id="c" width="1280" height="720" ></canvas>
</div>
)
}
export default App And the import logic works, while the app doesn't because we are full of side effects yet. We have 2 builds, one for ES and one from UMD, type works and then we can also transpile the SRC folder as it is for single file imports, but that shouldn't be considered a feature yet, but more something to experiment with. Is still strange that in order to import Text and Canvas i have to pull in both Rect and Group but eventually we still have some import to tweak here and there. |
Also at some point we shoud check esbuild seems like it does what rollup does but in a tenth of the time. |
|
||
// TODO: consider using https://github.com/swiing/rollup-plugin-import-assertions so we can import json in node and have rollup build pass | ||
export { version as VERSION } from '../package.json'; | ||
export const VERSION = version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a note about import assertion.
Looks like that the whole JSON has to be imported as a default import, or at least this is what was happening while i added the babel plugin to support import assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that the syntax must be like this. but babel...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is just the plugin i found that is not great.
If i use the good one, to support assertion, i get all the package.json in the codebase.
While this is a plugin that swaps the statement with a const, but has been poorly written
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, good job! This part is the hardest and most frustrating IMO.
I don't have too much to say accept that it is disappointing that there is so much trial and error in this field, I would expect rollup to explain all this in a few boilerplates. But here we are.
I don't understand, if I want to use fabric in a tree shakable manner I can't use the rollup outputs? Sounds ridiculous, why would anyone use rollup then? I can't believe it is true.
My main point to change in this PR are the index files, hopefully that will fix this issue.
We must not use const
, only import/export. That is why we should have more index files for utils, filters etc.
I am quite sure this is what is holding back tree shaking.
I also think it must be possible doing:
import {Canvas, Textbox} from 'fabric'
And have tree shaking work w/o importing files directly. That is the point of tree shaking, elsewise you are manually reducing the tree by manual imports. So that can't be considered as tree shaking. No need to support that IMO once tree shaking works.
I am also against exporting a default monolith, I thought you are against as well. Is this just a step forward? If a dev wants to use the fabric namespace they should do:
import * as fabric from 'fabric'
What is your opinion?
type TFabricEnv = { | ||
document: Document; | ||
window: Window; | ||
isTouchSupported: boolean; | ||
isLikelyNode: boolean; | ||
nodeCanvas: Canvas; | ||
jsdomImplForWrapper: any; | ||
copyPasteData: TCopyPasteData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should die in a PR I need to port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if different copy paste code make this unnecessary, great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, using the native event transferData
is the path forward
@@ -1640,7 +1640,8 @@ export class FabricObject< | |||
cloneAsImage(options: any): Image { | |||
const canvasEl = this.toCanvasElement(options); | |||
// TODO: how to import Image w/o an import cycle? | |||
return new Image(canvasEl); | |||
const ImageClass = classRegistry.getClass('image'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
|
||
// TODO: consider using https://github.com/swiing/rollup-plugin-import-assertions so we can import json in node and have rollup build pass | ||
export { version as VERSION } from '../package.json'; | ||
export const VERSION = version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird that the syntax must be like this. but babel...
There are 2 way to obtain partial imports.
The other way is to provide disjointed files and you import what you need, and each file follow its own dependency graph. On top of that the approach above can be still be applied. I do agree that we should not create consts in the export, that breaks tree shaking, and that can be fixed, but since all the rest is not ready now, i wouldn't bother. The official method of import stays The index filex are necessary but i don't want them to be used in the code files because then we have to guards for us using the It could also be that some weird export sytnax works too |
Tackling these:
this.controls = objectDefaultControls
Regarding the index files, ok. |
Running next project now fails |
Let's add the fabricjs-webpack changes in place for the basice react setup, and you can push there a folder with the next example? so we can test it. |
possible, it is the next app from #8135 Must use the cli or link fabric manually |
The class registry gets run when you import a file for that class. Prototype changes have the same issue, but those are fixed when we do that in the constructor. Assigning controls in the constructor is a no go because then you don't have anymore a control set but hundreds of them. Fixing the default values as first will clear some lines of code that are outside classes and will let us see better what is left. |
Didn't understand. Sorry.
If you assign the same |
Ahh so you think this is holding back tree shaking because of the assignment to the prototype? |
We could use a devops/CI person... lending a hand |
so the issue with Class Canvas {
}
classRegistry.setClass('canvas', Canvas); and then in the application import { Canvas } from '../canvas/Class'; The |
Sorry i m not being super clear with explanations. |
commit 90e42df Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:59:19 2023 +0200 fix(): object caching value workaround for SSR commit 1278696 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:58:54 2023 +0200 ssr test commit 6761606 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:57:38 2023 +0200 restore jsdom cleanup in static canvas due to FabricObject to canvas method commit 8ad5647 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:56:41 2023 +0200 Update object.js commit d29352e Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:47:36 2023 +0200 SSR changes commit c98d6e0 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:19:49 2023 +0200 prepare for SSR commit bb66394 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:05:27 2023 +0200 Update package.json commit 4a3d4aa Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 12:04:21 2023 +0200 Update rollup.config.mjs commit 74299c3 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 12:01:03 2023 +0200 cleanup commit 78bf675 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:52:34 2023 +0200 cleanup commit c18651c Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:47:43 2023 +0200 node stream methods commit b7e03ca Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:27:39 2023 +0200 Update rollup.config.mjs commit 46eb9fd Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:26:05 2023 +0200 working! commit a1534c4 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 10:26:49 2023 +0200 node build
|
This reverts commit 7d22d71.
try the following on branch
|
I am very excited to share that SSR works out of the box with #8632 tested in |
I have made progress with tree shaking and found the following:
Take a look at 8006244, it is a test for tree shaking. Tree shaking works for brushes for example. You can see that I didn't import brushes and the entire brushes code was removed. |
https://github.com/fabricjs/fabricjs-webpack/tree/master/next-app added the broken next-app here. |
commit 0631c6a Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 14:16:01 2023 +0200 rename files: `dist/fabric*` => `dist/index*` commit 7438d30 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 14:15:56 2023 +0200 Update tsconfig.json commit a496545 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 13:53:51 2023 +0200 Update tsconfig.json commit 8937a56 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 13:14:45 2023 +0200 rename node index file commit f3d5320 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 13:04:22 2023 +0200 canvas streaming methods commit 5fcd4b0 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 12:35:35 2023 +0200 better exports field commit b9c81a6 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 10:50:08 2023 +0200 enhance env tests commit e4fab60 Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 10:26:32 2023 +0200 Revert code splitting This reverts commit 8c04490. Revert "Update treeShakingOutput.mjs" This reverts commit 7054438. Revert "rename to index" This reverts commit 8829519. Revert "move index files" This reverts commit 08ba23d. Revert "Update util.ts" This reverts commit ed04657. Revert "organize controls index" This reverts commit 91b0c06. Revert "organize" This reverts commit da802c9. Revert "tree shaking test" This reverts commit 8006244. commit c2f8b5b Merge: 7054438 8a8904e Author: ShaMan123 <shacharnen@gmail.com> Date: Thu Feb 2 10:20:06 2023 +0200 Merge branch 'master' into node-build commit 8a8904e Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Thu Feb 2 02:13:14 2023 +0100 TS(): Testing fabric 6 package configuration and code splitting. Fix fabric object access (#8622) commit 7054438 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:51:34 2023 +0200 Update treeShakingOutput.mjs commit 8829519 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:40:22 2023 +0200 rename to index this is revertable by design commit 08ba23d Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:39:20 2023 +0200 move index files commit ed04657 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:37:28 2023 +0200 Update util.ts commit 91b0c06 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:35:10 2023 +0200 organize controls index commit da802c9 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:33:35 2023 +0200 organize commit 8c04490 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:29:22 2023 +0200 index files commit 8006244 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 14:25:14 2023 +0200 tree shaking test commit 200dcf8 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 00:19:30 2023 +0200 Update build.yml commit d2af7c7 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 00:10:28 2023 +0200 Update env.js commit 624b624 Author: ShaMan123 <shacharnen@gmail.com> Date: Wed Feb 1 00:02:19 2023 +0200 rm commit b697b4b Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 23:51:29 2023 +0200 restore dist commit a6caef7 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 23:16:03 2023 +0200 main entry for node commit 1a97d33 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 17:36:28 2023 +0200 node env cjs/mjs commit 95049f3 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 15:22:51 2023 +0200 rename commit d1ee15c Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 15:18:56 2023 +0200 Update types.ts commit e754bc6 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 14:35:34 2023 +0200 fix exports field commit 90e42df Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:59:19 2023 +0200 fix(): object caching value workaround for SSR commit 1278696 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:58:54 2023 +0200 ssr test commit 6761606 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:57:38 2023 +0200 restore jsdom cleanup in static canvas due to FabricObject to canvas method commit 8ad5647 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:56:41 2023 +0200 Update object.js commit d29352e Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:47:36 2023 +0200 SSR changes commit c98d6e0 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:19:49 2023 +0200 prepare for SSR commit bb66394 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 13:05:27 2023 +0200 Update package.json commit 4a3d4aa Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 12:04:21 2023 +0200 Update rollup.config.mjs commit 74299c3 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 12:01:03 2023 +0200 cleanup commit 78bf675 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:52:34 2023 +0200 cleanup commit c18651c Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:47:43 2023 +0200 node stream methods commit b7e03ca Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:27:39 2023 +0200 Update rollup.config.mjs commit 46eb9fd Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 11:26:05 2023 +0200 working! commit a1534c4 Author: ShaMan123 <shacharnen@gmail.com> Date: Tue Jan 31 10:26:49 2023 +0200 node build commit ce1874b Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:59:36 2023 +0100 fix tests to do not reference fabric commit 1f8583c Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:41:04 2023 +0100 try different commit edc36d0 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:35:11 2023 +0100 pretty and changelog commit 8a0638a Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:32:53 2023 +0100 fixed fabric access commit ddbf31e Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:29:44 2023 +0100 fixed fabric access commit f3ab5d0 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:10:51 2023 +0100 no unused deps commit 3b459fc Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Mon Jan 30 11:05:03 2023 +0100 progress; commit 887b288 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Sun Jan 29 17:50:15 2023 +0100 declaration are back commit 40af839 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Sun Jan 29 17:16:26 2023 +0100 where are my types commit 536b644 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Sat Jan 28 21:46:52 2023 +0100 different import commit 6c43775 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Sat Jan 28 19:40:10 2023 +0100 build is ok, types are not commit 8033843 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Sat Jan 28 19:40:01 2023 +0100 build is ok, types are not commit 248066b Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Fri Jan 27 01:06:26 2023 +0100 missin file commit 3fb6cd4 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Fri Jan 27 01:04:50 2023 +0100 remove build commit a59eb23 Merge: 3e9dad0 84b7a35 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Fri Jan 27 01:04:23 2023 +0100 Merge branch 'master' into v6-package-testing commit 3e9dad0 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Fri Jan 27 01:02:38 2023 +0100 save so far commit 9c01ab1 Author: Andrea Bogazzi <andreabogazzi79@gmail.com> Date: Tue Jan 24 13:32:57 2023 +0100 module config
…fabric object access (#8622)
Motivation
Try to package the library with modern standards
Some notes i took from here about main/module/browser
https://esbuild.github.io/api/#main-fields
IN theory from a react or bundled project we do:
or
Without any of those beeing tree-shakable for now.
And from browsers or cdn imports we refer to
And also i still have no idea if the main es build is supposed to be minified or not.
Changes
module
directive is an ES build named fabric.js.Gist
In Action