RFC: SSR testing for `@fluentui/react-components` #22788

layershifter · 2022-05-03T14:06:23Z

We are targeting first class support for SSR in @fluentui/react-components, #17893. The missing piece is automated testing to prevent our components from breaking due changes.

This RFC is focused on the implementation proposal for such test.

Rendered preview

codesandbox-ci · 2022-05-03T14:12:05Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7cb877f:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

fabricteam · 2022-05-03T14:12:37Z

📊 Bundle size report

_{🤖 This report was generated against 79bbe380acd510fc68cfbca260853c5ca635368b}

size-auditor · 2022-05-03T14:21:30Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 2af65fc29bde3dfac8177db7acef39075e28b264 (build)

ling1726 · 2022-05-04T14:30:58Z

rfcs/shared/build-system/ssr-testing.md

+- Start Chrome (via `puppeteer`)
+  - **Note**: the latest version of Puppeteer will be used to get latest Chrome. Existing version in our repo does not support even `:focus-visible` (that creates false positive errors)
+- Open a page (`index.html` file)
+  - **Note**: Webserver will not be used, a file will be opened via `file://`


will file:// support all OS ?

For the purposes of this test, is the behavior of a page loaded from file:// any different from one served by a webserver?

will file:// support all OS ?

It should work on every major platform (I tested Mac & Windows), https://en.wikipedia.org/wiki/File_URI_scheme

For the purposes of this test, is the behavior of a page loaded from file:// any different from one served by a webserver?

No.

spmonahan

Really like this proposal -- especially that it reuses all the existing Storybook stories.

spmonahan · 2022-05-04T23:30:14Z

rfcs/shared/build-system/ssr-testing.md

+  - A page will have configured interceptors to avoid loading resources (images) from CDN
+- Ensure that there are no errors, otherwise test fails
+  - **Note**: For this reason the test app will run with `process.NODE_ENV = 'development'`
+  - **Note**: _Probably_ there is sense to restrict the test to catch only SSR related errors (in prototype it fails also on missing React keys, for example)


I see why we might want to restrict to only SSR errors but it might be better to start by casting a wider net and fail on any error. Sure, the React keys example doesn't have anything to do with SSR but the fact that our story triggered the warning means the story wasn't demonstrating best practices so, imo, it's good that it failed even though that's not the explicit point of this test suite.

If we see lots of errors unrelated to SSR failing this test suite then I'd be in favor or restricting our failure modes.

but the fact that our story triggered the warning means the story wasn't demonstrating best practices so,

If our stories produce those kind of warnings that should be handled by logic that triggers production builds of those stories -> public-docsite-v9. Also these can/should be covered by lint.

I see why we might want to restrict to only SSR errors but it might be better to start by casting a wider net and fail on any error. Sure, the React keys example doesn't have anything to do with SSR but the fact that our story triggered the warning means the story wasn't demonstrating best practices so, imo, it's good that it failed even though that's not the explicit point of this test suite.

While I agree with this, I don't see that it's a goal of this test. As @Hotell it should be covered by a different tooling.

spmonahan · 2022-05-04T23:40:33Z

rfcs/shared/build-system/ssr-testing.md

+- Start Chrome (via `puppeteer`)
+  - **Note**: the latest version of Puppeteer will be used to get latest Chrome. Existing version in our repo does not support even `:focus-visible` (that creates false positive errors)
+- Open a page (`index.html` file)
+  - **Note**: Webserver will not be used, a file will be opened via `file://`


For the purposes of this test, is the behavior of a page loaded from file:// any different from one served by a webserver?

spmonahan · 2022-05-04T23:49:52Z

rfcs/shared/build-system/ssr-testing.md

+export const Stories = () => (
+  <SSRProvider>
+    <FluentProvider theme={teamsLightTheme}>
+      <TextTruncate />


What is a report for an SSR test going to look like?

In the example all the stories for a component are in a single test but it's possible that only one story triggers an SSR failure. If all the stories are grouped into a single test it could be frustrating to debug a failure.

thats good point. IMO we can provide a solution afterwards. the main point of this is that we are living in the dark in terms if things work or not. I'd rather have this in our pipelines ASAP and tweak detail later if it makes sense.

In the example all the stories for a component are in a single test but it's possible that only one story triggers an SSR failure. If all the stories are grouped into a single test it could be frustrating to debug a failure.

Reporting problems is a good point. I would agree with @Hotell that it's important to have gates first.

From the errors that I have seen React usually shows which component in a tree caused a failure. I will make this test runnable locally for sure, so it will be possible to check/debug issues locally without CI.

spmonahan · 2022-05-04T23:50:17Z

rfcs/shared/build-system/ssr-testing.md

+## Pros and Cons
+
+- 👍 It's fast (3 seconds to run the whole test)
+- 👍 We will reuse existing stories


Really like this part!

spmonahan · 2022-05-04T23:50:35Z

rfcs/shared/build-system/ssr-testing.md

+
+## Pros and Cons
+
+- 👍 It's fast (3 seconds to run the whole test)


Will it be possible to run this test suite locally while working on a component?

I don't see reason why it would not be possible? puppeter will be already installed on your machine. it will also download proper chrome binary when executing so everything should work as intended. What would be nice for local development would be ability to execute that ssr solely for component you're working on. but ATM taking into consideration the speed of running it for everything in one shot is making that a low prio.

Will it be possible to run this test suite locally while working on a component?

Yes, it will be something like yarn build && yarn test in that package. So no hot reload or something like, but it should be a fast loop.

but ATM taking into consideration the speed of running it for everything in one shot is making that a low prio.

Yeah, that's why I ignored scoping part 🐱

Hotell

🚀

rfcs/shared/build-system/ssr-testing.md

Hotell · 2022-05-06T11:52:53Z

rfcs/shared/build-system/ssr-testing.md

+export const Stories = () => (
+  <SSRProvider>
+    <FluentProvider theme={teamsLightTheme}>
+      <TextTruncate />


thats good point. IMO we can provide a solution afterwards. the main point of this is that we are living in the dark in terms if things work or not. I'd rather have this in our pipelines ASAP and tweak detail later if it makes sense.

Hotell · 2022-05-06T11:53:53Z

rfcs/shared/build-system/ssr-testing.md

+
+We will need to get both CommonJS & ESM bundles:
+
+- CommonJS is needed to avoid evaluation with `ts-node` or `babel-register`


not sure I completely understand the need of commonjs ? it is because that main.utils or there is some other reason. If that 's the only reason we can extract that to separate lib so it's reusable and can be loaded via path aliases as is.

To clarify, we are going to perform SSR of existing stories to get HTML page.

For that we need to have stories evaluated in Node environment, they contain JSX and ESM code that cannot be directly executed by Node. It's what Next.js and other frameworks also do: bundle ESM to send on a client & bundle CJS to get executed in Node.

Hotell · 2022-05-06T11:56:40Z

rfcs/shared/build-system/ssr-testing.md

+  - A page will have configured interceptors to avoid loading resources (images) from CDN
+- Ensure that there are no errors, otherwise test fails
+  - **Note**: For this reason the test app will run with `process.NODE_ENV = 'development'`
+  - **Note**: _Probably_ there is sense to restrict the test to catch only SSR related errors (in prototype it fails also on missing React keys, for example)


but the fact that our story triggered the warning means the story wasn't demonstrating best practices so,

If our stories produce those kind of warnings that should be handled by logic that triggers production builds of those stories -> public-docsite-v9. Also these can/should be covered by lint.

Hotell · 2022-05-06T11:59:08Z

rfcs/shared/build-system/ssr-testing.md

+
+## Pros and Cons
+
+- 👍 It's fast (3 seconds to run the whole test)


I don't see reason why it would not be possible? puppeter will be already installed on your machine. it will also download proper chrome binary when executing so everything should work as intended. What would be nice for local development would be ability to execute that ssr solely for component you're working on. but ATM taking into consideration the speed of running it for everything in one shot is making that a low prio.

layershifter · 2022-06-02T16:11:08Z

FYI: I worked on the actual implementation and found a real problem 💥 See #23371.

layershifter added the Type: RFC Request for Feedback label May 3, 2022

msft-fluent-ui-bot assigned GeoffCoxMSFT May 3, 2022

layershifter force-pushed the rfc/ssr-testing branch from 1532b7b to d693e1e Compare May 4, 2022 07:43

layershifter mentioned this pull request May 4, 2022

SSR support in @fluentui/react-components #17893

Closed

RFC: SSR testing for @fluentui/react-components

7cb877f

layershifter force-pushed the rfc/ssr-testing branch from d693e1e to 7cb877f Compare May 4, 2022 07:52

layershifter marked this pull request as ready for review May 4, 2022 07:52

layershifter requested review from Hotell and a team May 4, 2022 07:53

ling1726 reviewed May 4, 2022

View reviewed changes

spmonahan reviewed May 4, 2022

View reviewed changes

Hotell approved these changes May 6, 2022

View reviewed changes

spmonahan approved these changes May 31, 2022

View reviewed changes

This was referenced Jun 7, 2022

chore: create a package for SSR tests #23430

Merged

chore: add SSR tests (build command) #23411

Merged

chore: add SSR tests (test command) #23457

Merged

ling1726 approved these changes Jun 8, 2022

View reviewed changes

layershifter merged commit b6f9445 into microsoft:master Jun 8, 2022

layershifter deleted the rfc/ssr-testing branch June 8, 2022 11:09

marwan38 pushed a commit to marwan38/fluentui that referenced this pull request Jun 13, 2022

RFC: SSR testing for @fluentui/react-components (microsoft#22788)

c966e38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: SSR testing for `@fluentui/react-components` #22788

RFC: SSR testing for `@fluentui/react-components` #22788

layershifter commented May 3, 2022 •

edited

Loading

codesandbox-ci bot commented May 3, 2022 •

edited

Loading

fabricteam commented May 3, 2022 •

edited

Loading

size-auditor bot commented May 3, 2022 •

edited

Loading

ling1726 May 4, 2022

spmonahan May 4, 2022

layershifter May 31, 2022

spmonahan left a comment •

edited

Loading

spmonahan May 4, 2022

Hotell May 6, 2022

layershifter May 31, 2022

spmonahan May 4, 2022

spmonahan May 4, 2022

Hotell May 6, 2022

layershifter May 31, 2022

spmonahan May 4, 2022

spmonahan May 4, 2022

Hotell May 6, 2022

layershifter May 31, 2022

Hotell left a comment

Hotell May 6, 2022

Hotell May 6, 2022

layershifter May 31, 2022

Hotell May 6, 2022

Hotell May 6, 2022

layershifter commented Jun 2, 2022


		## Pros and Cons

		- 👍 It's fast (3 seconds to run the whole test)


		We will need to get both CommonJS & ESM bundles:

		- CommonJS is needed to avoid evaluation with `ts-node` or `babel-register`

RFC: SSR testing for @fluentui/react-components #22788

RFC: SSR testing for @fluentui/react-components #22788

Conversation

layershifter commented May 3, 2022 • edited Loading

codesandbox-ci bot commented May 3, 2022 • edited Loading

fabricteam commented May 3, 2022 • edited Loading

📊 Bundle size report

size-auditor bot commented May 3, 2022 • edited Loading

Asset size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spmonahan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hotell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Jun 2, 2022

RFC: SSR testing for `@fluentui/react-components` #22788

RFC: SSR testing for `@fluentui/react-components` #22788

layershifter commented May 3, 2022 •

edited

Loading

codesandbox-ci bot commented May 3, 2022 •

edited

Loading

fabricteam commented May 3, 2022 •

edited

Loading

size-auditor bot commented May 3, 2022 •

edited

Loading

spmonahan left a comment •

edited

Loading