Skip to content
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): export util types #8915

Merged
merged 9 commits into from May 14, 2023
Merged

chore(TS): export util types #8915

merged 9 commits into from May 14, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented May 13, 2023

Motivation

More type exports

Description

Changes

  • move src/util/fireEvent => src/controls/fireEvent
  • rename src/util/type => src/controls/typeAssertions

Gist

In Action

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about * exports?
You said you are against, right?

Comment on lines +17 to 18
export type { IntersectionType } from './src/Intersection';
export { Intersection } from './src/Intersection';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export type { IntersectionType } from './src/Intersection';
export { Intersection } from './src/Intersection';
export * from './src/Intersection';

@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2023

Build Stats

file / KB (diff) bundled minified
fabric 900.600 (+0.079) 303.961 (0)

@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2023

Coverage after merging export-types1 into master will be

83.64%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 29, 32, 35
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%10
   Intersection.ts100%100%100%100%
   Observable.ts87.23%85.29%84.62%89.36%144–145, 170–171, 39–40, 48, 57, 91, 99
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/Pattern
   Pattern.ts92.21%91.89%90%93.33%116, 127, 136, 29, 92
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%107, 107, 107, 109, 111, 113–115, 117–120, 127–128, 135, 137, 22–23, 31–35, 39–43, 50–53, 61–65, 67, 75, 75, 75, 75, 75–76, 78, 78, 78–81, 83, 91–92, 94, 96–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.01%82.35%100%93.75%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%106, 106, 106, 106, 106–107, 109–110, 117–118, 120, 122–126, 135, 139–140, 140, 148, 148, 148–151, 153–156, 16, 160–161, 163, 165–168, 17, 171, 178–179, 181, 183–184, 186, 19, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 21, 21, 211, 22–23, 27, 36, 43, 50, 57, 64, 83–85, 93–95, 97–98
src/canvas
   Canvas.ts78.87%77.54%81.67%79.41%1001–1002, 1002, 1002–1004, 1006–1007, 1007, 1007, 1009, 1017, 1017, 1017–1019, 1019, 1019, 1025–1026, 1034–1035, 1035, 1035–1036, 1041, 1043, 1074–1076, 1079–1080, 1084–1085, 1198–1200, 1203–1204, 1277, 1396, 1519, 1589, 162, 187, 297–298, 301–305, 310, 333–334, 339–344, 364, 364, 364–365, 365, 365–366, 37, 374, 379–380, 380, 380–381, 383, 392, 398–399, 399, 399, 41, 442, 450, 454, 454, 454–455, 457, 539–540, 540, 540–541, 547, 547, 547–549, 569, 571, 571, 571–572, 572, 572, 575, 575, 575–576, 579, 588–589, 591–592, 594, 594–595, 597–598, 610–611, 611, 611–612, 614–619, 625, 632, 669, 669, 669, 671, 673–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 742, 742, 800–801, 801, 801–802, 804, 807–808, 808, 808–809, 811–812, 815, 815–817, 820–821, 891, 903, 910, 931, 963, 984–985
   SelectableCanvas.ts94.39%91.16%94.55%96.63%1119, 1119–1120, 1123, 1143, 1143, 1201, 1254–1255, 1276, 1284, 1409, 1411, 1413–1414, 518, 698–699, 701–702, 702, 702, 751–752, 813–814, 867–869, 901, 906–907, 934–935
   StaticCanvas.ts96.85%92.86%100%98.61%1118–1119, 1119, 1119–1120, 1240, 1250, 1304–1305, 1308, 1343–1344, 1421, 1430, 1430, 1434, 1434, 1481–1482, 326–327, 344, 775, 787–788
   TextEditingManager.ts100%100%100%100%
src/color
   Color.ts92.16%86.49%100%94.29%314–315, 319–320, 323–324, 42, 72–73, 73, 75, 75, 75–76, 78–79
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   Control.ts93.90%88.89%90.91%97.73%232, 319, 319, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts5.97%0%0%11.11%100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%130–131, 162–163, 170, 176, 178
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%77.78%50%100%14, 17
   index.ts100%100%100%100%
   node.ts73.08%33.33%66.67%88.24%27, 31–32, 32, 32, 43
src/filters
   BaseFilter.ts22.04%23.21%32%19.05%100, 100, 100–101, 108–111, 111, 111–112, 118, 118, 118–121, 139, 157, 171–176, 180–181

Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at src/util/misc/svgParsing.ts
What should be exported etc.?
Please decide about enums. Do we want/need them?
IMO string types are more straight forward so I don't see a need for them.

@ShaMan123 ShaMan123 requested a review from asturur May 13, 2023 07:59
@ShaMan123 ShaMan123 changed the title chore(TS): export types chore(TS): export util types May 13, 2023
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you disapprove * exports I will refactor

@ShaMan123
Copy link
Contributor Author

Feel free to commit changes

@asturur
Copy link
Member

asturur commented May 14, 2023

What about * exports? You said you are against, right?

I m not against, i would like to divide exports of types from the one of code.
That can be done with a typedef file, a named export type, or 2 export from the same file

@asturur
Copy link
Member

asturur commented May 14, 2023

Trying to figure out if again code bloat is because of what.
I can't see any new code

Still the bundle grows. New types shouldn't even introduce new variables that could lead to move some commonly used variable from 2 letter to 3 letter because we finished the combinations of 2.

How this stuff works?

@asturur
Copy link
Member

asturur commented May 14, 2023

ok we are exporting some new stuff as code that shouldn't be part of the interface probably. i m not sure.

Comment on lines +69 to +72
export type {
EnlivenObjectOptions,
LoadImageOptions,
} from './misc/objectEnlive';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here is where i start to have some doubts.
How do i access those types then? Are they on the main export root or do i need and grab them from inside util?

@asturur
Copy link
Member

asturur commented May 14, 2023

For how we are use enums, enums are useless.
I don't think they are supposed to be compared to string unions, but they need to replace unreadable code.

So for example:

const enum configurationBits = {
  vgaScaler: 1,
  hdmi96k: 2,
  usbHub: 4,
  stereomode: 8,
  syncOnGreen: 16,
}

So that then in the code you can use

configurationBits.vgaScaler & configurationBits.hdmi96k

And then you know that at compilation time it goes back to

1 & 2

That we would consider unreadable and would make every configuration that uses numbers the same to the other.

Enums are more straight forward to use to me compared to strings union, but are the wrong tool for the job to pick up from a choice of strings, so we shouldn't use them to differentiate between jpg/png for example.

I don't think we have a valid use case for enums in our codebase right now

@asturur
Copy link
Member

asturur commented May 14, 2023

@ShaMan123 do you think it make sense ot use the type or typedef file as index file for types?

Should types be all first level exports even if we have utils and controls objects exports?

@asturur
Copy link
Member

asturur commented May 14, 2023

so the extra code was indeed us exporting the enums as code.
And that is the reason why i prefer to have types separated from code in import / exports.
We want to export every type we add, we don't always want to export all the code because we don't want to support anything we export once and keep it there because then is breaking changes.

@asturur
Copy link
Member

asturur commented May 14, 2023

let's merge as it is, since it has no bad side effects.
But i think when we have all types exported i want to give a stab at proposing a more strict organization, with types all at first level exports.

@asturur asturur merged commit 0f0ed96 into master May 14, 2023
18 checks passed
@asturur asturur deleted the export-types1 branch May 14, 2023 08:44
@ShaMan123
Copy link
Contributor Author

@ShaMan123 do you think it make sense ot use the type or typedef file as index file for types?

Should types be all first level exports even if we have utils and controls objects exports?

I am not sure about top level type exports. IMO it makes sense to have types at the same level as what they are describing. For discovery.
However, I am not sure how to import a type nested under a named export like util. Can I spread the import and import it directly? If not, it is inconvinient so I would prefer top level exports it that case or expose utils as an entry point 'fabric/util'. Actually that might be the best solution.
Entry points for

  • shapes
  • Brushes
  • Util
  • Controls
  • Filters
  • Etc.

But then we need to think how 'fabric/node' fits into this pattern.

What are your thoughts and preferences?

@ShaMan123
Copy link
Contributor Author

so the extra code was indeed us exporting the enums as code.
And that is the reason why i prefer to have types separated from code in import / exports.
We want to export every type we add, we don't always want to export all the code because we don't want to support anything we export once and keep it there because then is breaking changes.

This is a good point.

@ShaMan123
Copy link
Contributor Author

But i think when we have all types exported i want to give a stab at proposing a more strict organization, with types all at first level exports.

Ok.

@ShaMan123
Copy link
Contributor Author

Thinking a bit more about entry points.
If we go in that direction we can split fabric into submodules. Even in npm. That might prove to be good. And also promote plugins etc.

@asturur asturur mentioned this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants