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(): Update canvas and jsdom #10417

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

chore(): Update canvas and jsdom #10417

wants to merge 4 commits into from

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 26, 2025

Description

The main update here is canvas 3.x hopefully is easier to handle in the cd/ci

Copy link

codesandbox bot commented Jan 26, 2025

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

Build Stats

file / KB (diff) bundled minified
fabric 923.313 (0) 306.732 (0)

import { ModuleMocker } from 'jest-mock';
import { installCommonGlobals } from 'jest-util';

// The `Window` interface does not have an `Error.stackTraceLimit` property, but
Copy link

@Smrtnyk Smrtnyk Jan 26, 2025

Choose a reason for hiding this comment

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

Have you considered on moving to vitest for unit tests and executing tests in real browser?
that way you can use real canvas for testing and not fake jsdom one
https://vitest.dev/guide/browser/
vitest has jest compatible API so I guess it wouldn't be that much work migrating unit tests

btw jest isn't seeing much love lately, it is barely maintained and I am questioning its future to be honest

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that 70% of our unit tests are still on QUnit.
Then we have playwright.
Ideally unit tests won't run in the browser and will just test the fabricJS code and not require a real canvas, but just a mocked one.
Visual tests will run both in the browser and node throught playwright and would be based on browser or JSDOM + canvas and test also output of the canvas.

Vitest should be api compatibile with jest, and easy replacement, but before doing anything like that i would like to see qunit replaced.

So what we have now is a middle ground because there is no firepower to rewrite all the unit tests

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear if you want to open a PR to swap jest with vitest and and have it running in node only for now, you are welcome to do so, as long as it can support canvas with JSDOM is all good

Copy link

Choose a reason for hiding this comment

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

To be clear if you want to open a PR to swap jest with vitest and and have it running in node only for now, you are welcome to do so, as long as it can support canvas with JSDOM is all good

I am interested in this so will have a look at this on the weekend

Copy link
Contributor

Coverage after merging canvas-jsdom into master will be

84.98%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
fabric.ts86.92%20%0%95.79%10, 144, 144, 144, 144, 144, 190, 190, 190, 190, 190, 31, 33, 33, 33, 33, 33–34, 34, 34, 34, 34, 39, 8
index.node.ts14.29%100%0%25%23, 26, 29, 41, 44, 47
extensions/aligning_guidelines/util
   basic.ts100%100%100%100%
extensions/data_updaters/origins
   index.ts97.50%75%100%100%22–23
src
   ClassRegistry.ts91.18%64.29%100%97.92%35, 56
   Collection.ts72.55%33.85%51.61%84.67%130, 153, 155–157, 159, 167, 169–170, 181, 197, 217, 228, 241, 243, 245, 252, 254, 263, 265, 270, 279, 281, 286–287, 302, 304, 306, 309–310, 327–328, 328–329, 333–334, 337–338, 338–344, 346–348, 350, 75, 99
   CommonMethods.ts88.57%64.29%83.33%96%50, 52, 9
   EventTypeDefs.ts100%100%100%100%
   Intersection.ts81.42%41.30%73.33%96.14%105, 132–133, 166, 184–190, 228, 228, 237, 239, 289–291, 297, 297, 78, 80
   Observable.ts72.25%44.19%31.25%86.36%144, 153, 156, 168, 170, 172, 175, 41, 43, 68–70, 72, 76, 78, 80, 82–85, 87–91
   Point.ts84.95%62%44.12%92.68%104, 117, 148, 157, 179, 197, 206, 216, 225, 236–239, 259, 284–285, 294–297, 328, 341, 349, 359, 95
   Shadow.ts80.91%13.04%37.50%90%139, 139–140, 140–141, 143, 145–150, 159, 176, 176, 176, 176, 194, 194, 196–197, 197, 199, 206, 206, 223–230, 232, 232, 234–235
   cache.ts83.72%45.45%75%90.14%57, 59, 71–72, 74–77
   config.ts82.63%20%44.44%94.20%127, 135–136, 138–141, 143, 146–147, 151, 156, 156, 156, 156, 156, 156
   constants.ts100%100%100%100%
   typedefs.ts100%100%100%100%
src/LayoutManager
   ActiveSelectionLayoutManager.ts86.67%76.92%28.57%100%
   LayoutManager.ts89.25%65.48%53.85%99.31%279, 344, 355
   constants.ts100%100%100%100%
   index.ts48.57%37.50%80%66.67%1, 1, 1–2, 2, 2–3, 3, 3–4, 4, 4–5, 5, 5, 5–6
   types.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts71.05%37.50%100%78.95%39, 41–44, 46–48, 54, 57–58, 66–69
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts85.71%20%100%100%23, 23
   LayoutStrategy.ts86.78%57.14%71.43%97.67%54, 73, 75, 88
   utils.ts62.90%7.14%100%78.72%28–32, 34–35, 39, 39–44, 47, 47, 47
src/Pattern
   Pattern.ts52.08%2.86%10%63.08%105–107, 113, 113–114, 117, 117–119, 119, 119–122, 130, 130–132, 132, 132, 132, 132, 132–138, 140–141, 143, 150, 150, 150, 150, 153–160, 160, 160–164, 174–175, 175, 175, 175–176, 176, 176–182, 182, 182, 182–183, 183, 183–188, 190–199, 203–206, 209–210, 212–214, 216–218, 218, 218–221, 33, 37
   index.ts50%100%100%50%2
   types.ts100%100%100%100%
src/brushes
   BaseBrush.ts78.67%8.33%14.29%88.55%110, 114, 114–115, 122, 122, 122, 122, 124–125, 130, 135, 143, 146, 155, 155, 155, 155, 155–160, 99
   CircleBrush.ts52.10%12.50%12.50%58.25%100–108, 108–118, 122, 130–139, 55, 67, 69, 76, 76, 78–79, 79, 83, 85–86, 92–98
   PatternBrush.ts43.48%0%0%53.57%21, 21, 25–34, 36, 43, 43–44, 51–53, 53, 53–54, 60–64, 64, 64–69
   PencilBrush.ts40.14%0%0%52.02%103, 103, 116, 122, 122, 124, 129, 138–139, 146, 146, 146–147, 147, 147–152, 152, 152, 152, 152, 155–158, 167–169, 176, 176, 176, 176–177, 181–185, 185, 185, 185, 185, 188–205, 215, 224–232, 232, 235–238, 244, 244, 244–257, 257, 257–266, 274–276, 276, 276–280, 280, 280–299, 54, 54, 68, 68, 71, 71, 71–72, 75, 84, 84, 87, 87–88, 88, 88, 88, 91, 91, 91, 91–92, 92
   SprayBrush.ts51.67%0%0%58.86%100, 107, 107, 107, 107, 107–112, 118–120, 122, 128–135, 137–139, 141, 141, 141–148, 148, 152, 157, 162, 164, 169–170, 172, 180, 182, 185, 187, 195, 198–200, 200, 200–207, 207, 207–210, 212, 22, 22, 67, 85–87, 94–99
   typedefs.ts100%100%100%100%
src/canvas
   Canvas.ts74.18%32.78%87.50%86.20%1012–1013, 1016, 1018, 1020–1021, 1024–1025, 1030, 1035, 1049, 1066, 1080–1085, 1094, 1119–1120, 1141, 1143, 1145, 1152, 1155, 1157, 117, 1218–1222, 1256, 1256, 1264, 1267, 1300–1301, 1301–1305, 1308, 1329, 1329, 1332–1333, 1359, 1367, 1392, 1394–1429, 1433, 1435–1436, 1438–1440, 1444, 1446, 1448–1450, 1453–1458,

@asturur
Copy link
Member Author

asturur commented Jan 26, 2025

This requires bumping up node from 16 to 18, meaning fabric 7.0 which sound painful as of today for me

@asturur
Copy link
Member Author

asturur commented Jan 26, 2025

maybe i should call it 7.0 and move on, who cares.

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.

2 participants