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

Updated rollup dependencies #8557

Closed
wants to merge 8 commits into from
Closed

Updated rollup dependencies #8557

wants to merge 8 commits into from

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 1, 2023

Motivation

This update seems to have no side effects on the build
Just a reference branch to experiment with using exports

Description

Changes

Gist

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Build Stats

file / KB (diff) bundled minified
fabric 932.057 (0) 297.225 (0)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Coverage after merging updated-dependencies into master will be

83.37%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.js35%12.50%0%54.55%430, 432, 432, 432, 432, 432, 434–435, 437, 437, 437–438
src
   cache.ts96.97%90%100%100%56
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   env.ts72.73%53.33%100%79.17%22, 22–23, 23, 23, 25, 25, 27, 29, 31–32, 62
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%118, 124, 135, 144, 96
   point.class.ts100%100%100%100%
   shadow.class.ts98.48%96%100%100%156
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts0%0%0%0%100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 134, 141, 143, 23, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
   pattern_brush.class.ts97.06%87.50%100%100%21
   pencil_brush.class.ts91.86%85.42%100%93.58%122–123, 152, 152–154, 276, 280, 285–286, 68–69, 84–85
   spray_brush.class.ts0%0%0%0%100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 77, 89–91, 99
src/canvas
   canvas.class.ts92.78%88.52%94.12%95.70%1154, 1154–1155, 1158, 1178, 1178, 1240, 1276–1277, 1296–1297, 1305–1306, 1327, 1335, 1448–1449, 1451–1452, 1472–1473, 1636–1637, 1641, 510, 577–578, 583, 593, 722–723, 725–726, 726, 726, 772–773, 834–835, 888–890, 920, 925–926, 955–956
   canvas_events.ts78.19%75.30%82.81%79.48%1003–1004, 1004, 1004–1006, 1008–1009, 1009, 1009, 1011, 1019, 1019, 1019–1021, 1021, 1021, 1027–1028, 1036–1037, 1037, 1037–1038, 1043, 1045, 1075–1077, 1080–1081, 1085–1086, 1184, 1204–1206, 1209–1210, 1282, 1402, 145, 1496–1497, 1503, 1507–1508, 1524, 1546, 1593, 1598, 1637, 170, 318–319, 319, 319–320, 320, 323–327, 332, 334, 346–348, 351, 368, 368, 368–369, 369, 369–370, 378, 383–384, 384, 384–385, 387, 396, 402–403, 403, 403, 439, 443, 443, 443, 443, 443–444, 446, 521, 523, 523, 523–525, 545, 547, 547, 547–548, 548, 548, 551, 551, 551–552, 555, 564–565, 567–568, 570, 570–571, 573–574, 586–587, 587, 587–588, 590–594, 600, 607, 647, 647, 647, 649, 651–655, 661, 667, 667, 667–668, 670, 673, 678, 691, 718, 774–775, 775, 775–776, 778, 781–782, 782, 782–783, 785–786, 789, 789–791, 794–795, 805, 887, 901, 908, 929, 961, 982–983, 989
   static_canvas.class.ts94.56%88.89%97.92%97.12%1128–1129, 1129, 1129–1130, 1250, 1260, 1314–1315, 1318, 1353–1354, 1432, 1441, 1446, 1495–1496, 1724, 1724–1725, 1775, 1778, 1781, 1781, 1781, 1784, 1787, 1787, 1787, 309, 345, 358, 414–415, 417–418, 427, 431–432, 435–436, 888
src/color
   color.class.ts92.16%86.49%100%94.29%330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   changeWidth.ts100%100%100%100%
   control.class.ts93.90%88.89%90.91%97.73%235, 319, 319, 354
   controls.render.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   default_controls.ts86.67%66.67%100%100%122, 129
   drag.ts100%100%100%100%
   rotate.ts20%12.50%50%22.22%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.41%92.68%100%93.59%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/filters
   2d_backend.class.ts92%83.33%100%93.75%35–36
   FilterBackend.ts88.89%88.89%100%85.71%15–16
   WebGLProbe.ts40.54%40%60%36.36%28–30, 30, 30–31, 33–35, 43, 46–48, 48, 48–51, 53, 58
   base_filter.class.ts20.83%20.83%33.33%18.18%102–104, 104, 104–105, 112–115, 115, 115–116, 122, 122, 122–125, 143, 173–178, 182–183, 183, 183–186, 186, 186,

"rollup": "^2.75.6",
"rollup-plugin-size-snapshot": "^0.12.0",
"rollup-plugin-terser": "^7.0.2",
"rollup": "^3.9.0",
"rollup-plugin-ts": "^3.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

what about the ts plugin?

Copy link
Member

Choose a reason for hiding this comment

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

I saw you added and then removed it...
You must update typescript as well I believe

Copy link
Member

Choose a reason for hiding this comment

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

Ohh I see that is the cause of the major fuck up in classes

@asturur
Copy link
Member Author

asturur commented Jan 3, 2023

Should i continue on this PR or move to more TS finishing?

@ShaMan123
Copy link
Member

whatever you think
we should manage to work with rollup, we are currently hacking it

@asturur
Copy link
Member Author

asturur commented Jan 5, 2023

I think i understood the code change issue.
When we do in typescripy

Class A {
  propertyB: number;
}

without giving it a full value, either that gets removed from the traspiler, or it gets initialized to void.

Initializing to void override whatever is on the prototype.

Using declare solve the issue, but the older plugin somehow knew how to handle it on its own probably because it deferred to TS for JS emitting.

I think the safe approach, while boring, is to add declare in front of those, because that is what we are actually doing, declaring that the value will be there.

@asturur
Copy link
Member Author

asturur commented Jan 5, 2023

That will be boring and long, since it requires to change all the classes again, and temporary since we need to fix the default values anyway. So i shouldn't waste time on that but rather go straight to the default value issue

@asturur
Copy link
Member Author

asturur commented Jan 5, 2023

On top of that i don't like our declarations are emitted right now.
typescript isn't bundling the code of course, so is not emittin a single declaration file but many of them ( one per file ) and i don't see anything understanding that once the file is bundled.

@@ -1,3 +1,2 @@
# Browsers that we support

chrome >= 58
chrome >= 100
Copy link
Member Author

Choose a reason for hiding this comment

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

just trying around, not supposed to change this file

}
export default fabric;

export { fabric };
Copy link
Member Author

Choose a reason for hiding this comment

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

i m really not sure all this mess is worth for this change tho.

Comment on lines +79 to +87
"@babel/plugin-transform-typescript": "^7.20.7",
"@babel/preset-env": "^7.20.2",
"@babel/preset-typescript": "^7.18.6",
"@rollup/plugin-babel": "^6.0.3",
"@rollup/plugin-json": "^6.0.0",
"@rollup/plugin-terser": "^0.2.1",
"@rollup/plugin-typescript": "^10.0.1",
"@swc/cli": "^0.1.59",
"@swc/core": "^1.3.24",
Copy link
Member Author

Choose a reason for hiding this comment

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

we are not keeping all of those, searching for a simple subset

@ShaMan123
Copy link
Member

A different approach:
Since all browsers support the module type script tag and allow imports why should we mess around eith building.
Just TS and be done

@asturur
Copy link
Member Author

asturur commented Jan 5, 2023

Can you link me to the module script tag specs? i don't understand which feature you refer to.

@asturur
Copy link
Member Author

asturur commented Jan 5, 2023

@ShaMan123
Copy link
Member

@asturur
Copy link
Member Author

asturur commented Jan 8, 2023

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules

Oh ok the MJS browser modules.
I m NOT gonna pioneer that thing.
is unclear how those files get transferred? are those multiple network connections?
is unclear how do you get the files at all with NPM et similars.
is unclear if devs need to repack them anyway because they have to bundle
What happens if the network of dependencies becomes a huge mess?

I m also not ready to support it. If someone makes me a question i have to go and study.

I want to do canvas and drawing not pioneer js stuff.
Babel and TS are already a stretch for me

The old string concatenation build was awesome now that i look back.
Fast. modular. worked. lasted 12 years.

@asturur asturur closed this Jan 10, 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