-
-
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
cd(): Surface the minified build as standard when importing and also provide a non bundled build #9624
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Build Stats
|
Need to hook it up to a real app to test differences, but seems the part i was missing in my head. |
if this works as i want it to work we can add more files and simply not include them in fabric.ts but in a different ts file and have them available without influencing the core bundle |
@ShaMan123 i have to test this, but this would allow to do something like: import { Canvas, Text } from 'fabric/es' And then your bundler would need to bundle only the stuff that is really necessary for the 2. It wouldn't be as byte optimized as a tree shaking software that goes around the bundled build and chunks off pieces of code that are not needed, but in general would work I have a small app in development so maybe tomorrow i m able to test this |
Now this can't be the default of course because if i m using either svgImport or loadCanvasFromJson with an app that requires a shape, like Path that i m not requiring in my code, that is going to break the app. So this this /es/ folder should be kept for experimentation or 'i know what i m doing' for now and node should be left to a bundled output |
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.
Don't we want es to become the default export instead of the bundled one?
Yes. Probably the solution will be that utils like loadSVG will force import all classes, so that when you load them all the imports are resolved, and loadFromJSON instead of being a canvas method will be a util as well, so that when you trigger generic code you are forced to import all the dependencies. This could leave freedom to people to not do that anyway by provide a clear UNSAFE_loadSVGFromString import if they know what they want to do. The approximate goal is to be able to discard large chunks:
|
I would even set a goal to make them subpackages |
"import": "./dist/index.mjs", | ||
"require": "./dist/index.js", | ||
"default": "./dist/index.js", | ||
"import": "./dist/index.min.mjs", |
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.
should not be minified so it seems
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 there is something wrong with this change open a visible issue, comments on old PR have basically no visibility unless i m combing the emails one by one.
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.
@asturur The non-minified build is basically never picked up. This should have been done like React, where it picks the correct one based on the ENV:
Forcing the minified build during development is just more painful for debugging. You can also reuse the same strategy to strip fabric's logs for the production build.
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.
And actually it breaks jest snapshots because class names are now mangled in the test
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.
important is that production has the minified build, if you want top open a change to surface the non minified build in dev you can do it.
Surfacing non minified code in production seems as bad as minified in dev.
Regarding the affected test, what snapshot is it about?
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.
lodash, redux, ships unminified, lottie player ships unminified. Those are random picks i made.
Important is that there is something for the browser inclusion that is still minified.
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.
On a look at rollup.config.mjs
, I don't know exactly who the fabric build system works so I'll leave the PR to you guys if you want to do it. Otherwise no problem, we can wait if someone else has issues. Internally at my workplace we've just patched package.json
to continue using the unminified build.
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.
But do sourcemaps works for you or not?
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.
node_modules sourcemaps work fine for me with CRA, but not with Vite for some reason vitejs/vite#13391 (despite the issue being supposedly fixed but I'm not an expert with Vite).
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.
I opened this PR because my VITE build was picking up the unminified code, so vite wasn't minifying anything for me
Description
I was looking at the bundle analyzer tool of one app and realized we are always importing the 900kb file
Also enabled source maps for everything.
Then i understood there is another option to build the files and that is by preserving the module import structure and renaming the outputs to .mjs so that the basic standard import commands can work