-
-
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
chore(TS) use export to get type definitions #8585
Conversation
@ShaMan123 remade this PR from scratch since the other branch was tainted from experiments |
Build Stats
|
Oh yes of course now we need something that set fabric into the window to properly work when added as a script i assume |
Well great :) it works either on browsers or on node |
* Cached array of text wrapping. | ||
* @type Array | ||
*/ | ||
declare __cachedLines: Array<any> | null; |
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.
pointed out this was unused.
compared to master we are saving around 2000 bytes here, some babel magic probably.
|
@ShaMan123 that's it. If we want export and type declaration this is the PR We need a follow up PR that removes more undefind class properties and then we should start moving the defaults in the class. |
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.
Shouldn't we add more fields to the package for exports and types?
file: process.env.BUILD_OUTPUT || './dist/fabric.js', | ||
name: 'fabric', | ||
format: 'umd', | ||
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.
I read that maybe we should add
exports: 'named'
Yes we don't need to do all now. But yes we need to tell package.json where types are at least |
export named doesn't make much sense, but if is possible we could do it. |
I agree, or |
Ok merging and reiterating soon with some code shaking test |
Motivation
Get out of our transition type build definitely.
Description
I had to switch to babel after i was burned by the ts-plugin experience.
Babel output more errors and is more strict and more configurable.
In the configuration of rollup there are also some new bits about export d.ts files that will help us emit type declaration that mirror the interface without exporting internal types.
The build directory now has a src folder that contains type definition for each class and utils.
Just a note on class properties:
If we pick up a transpile target that does not support public properties ( likely to be true ) this is how they should look like and also this is how we should define them when creating them from default values.
The _defineProperty function is very small and we should use the same, but i don't think we can force babel then to recognize our.
We could either keep them different or move the public property to manually defined values in the constructor for the sake of consistency.
I don't have a strong opinion as long as defaults are configurable.
Speakin of supported JS and code
canvas?.viewportTransform
becomescanvas === null || canvas === void 0 ? void 0 : canvas.viewportTransform
And i hate it.
I highly prefer
canvas && canvas.viewportTransform
Changes
import fabric from 'fabric'
rather thanimport { fabric } from 'fabric'
( not a choice appartenly )Follow ups
Looking in the built file for
, void 0);
will identify missed declared properties ( still many ) and they need to be cleaned upIn Action