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

test(): add SSR test #9490

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

test(): add SSR test #9490

wants to merge 16 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Nov 10, 2023

Motivation

The recent SSR discussions and bugs related to env convinced me we need a test that shows SSR works so we can change env stuff with confident

Description

I created a test using the next template

Changes

I had to make the next template emit a signal so the test knows when the server/app is ready so I added logic to the template (instrumentation hook)
I adapted the cli start cmd by adding flags so that the browser and vscode launch only if flagged + added a port flag.
Added tsconfig to the vanilla app to fix a parcel ts bug
Fixed node template so that after deploying it works (node version mismatch)

Gist

In Action

Copy link
Contributor

github-actions bot commented Nov 10, 2023

Build Stats

file / KB (diff) bundled minified
fabric 904.508 (0) 303.989 (0)

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.

Ready to merge, might seem a lot of changed files but most are minor patches


test.beforeAll(({}, testInfo) => {
testInfo.setTimeout(60 * 1000);
task = exec(`npm start next -- -p ${PORT} --no-watch`, {
Copy link
Contributor Author

@ShaMan123 ShaMan123 Nov 10, 2023

Choose a reason for hiding this comment

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

once playwright/fabric are mjs then we can import startSandbox directly instead of spawning this cmd

await page.getByRole('textbox').press('Control+a');
await page
.getByRole('textbox')
.fill("fabric supports SSR!\nIsn't that amazing?!");
Copy link
Member

Choose a reason for hiding this comment

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

This message is confusing.
What fabric support is only not blowing up node with an import that run browser code.
How do we ensure that the page was actually rendered by server?
Imho the test can just curl the output of http://localhost:${PORT}/ and verify it contains the snapshot of html.
The fact that the page load could also simply mean that SSR isn't active, no?

I would also change fabric supports SSR!\nIsn't that amazing?! in a softer a page that uses fabricJS will render ok with framework that use SSR

Copy link
Contributor Author

@ShaMan123 ShaMan123 Nov 11, 2023

Choose a reason for hiding this comment

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

How do we ensure that the page was actually rendered by server?

It doesn't, does it?
That is truly supporting SSR.
It is confusing saying the current state is SSR support, you are right.
I mean you can import fabric in SSR and it will not break. It will run on the browser after dehydration.
It is interesting to support SSR by rendering on node and sending it as an image or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the test I just want to prove fabric works after window loads

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.

applied minors

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.

fixed a parcel bug and rebased
this is good to go and should merge ASAP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes parcel class bug
parcel-bundler/parcel#839 (comment)

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.

a few more crucial CD fixes

@@ -0,0 +1 @@
FROM node:18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes deployed sandbox node version
our package.json require >16.20 I think so this fixes that
codesandbox/codesandbox-client#1469 (comment)

.codesandbox/start.mjs Outdated Show resolved Hide resolved
@ShaMan123 ShaMan123 mentioned this pull request Nov 19, 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.

finished

Update start.mjs
Copy link
Contributor

Coverage after merging ssr-test into master will be

82.75%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   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.31%91.89%90%93.55%119, 130, 139, 32, 95
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79%76.99%83.05%79.84%1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.21%91.60%94.44%94.22%1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
   StaticCanvas.ts96.78%93.09%100%98.53%1019, 1029, 1081–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts89.58%83.33%91.67%91.67%16–17, 17, 17
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 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.ts7.25%0%0%13.51%103, 108, 120, 120, 120, 120, 120, 122–125, 125, 128, 135, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
   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%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env
   browser.ts84.21%<

@ShaMan123 ShaMan123 added the stale Issue marked as stale by the stale bot label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD stale Issue marked as stale by the stale bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants