[ARGG-876] Add support for withScrimmedPortal with SSR #3085

anambl · 2023-11-15T18:34:58Z

The WithScrimmedPortal BPK component is not compatible with SSR due to the use of the React Portal. To be able to use the component in projects that use SSR, we should provide a fallback behaviour that can be rendered on the server. The fallback behaviour should be the BpkScrim component, however, in the future we can allow consumers to pass another component if needed.

Remember to include the following changes:

README.md (If you have created a new component)
Component README.md
Tests
Storybook examples created/updated
For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

github-actions · 2023-11-15T18:38:51Z

Visit https://backpack.github.io/storybook-prs/3085 to see this build running in a browser.

runmoore · 2023-11-16T09:33:23Z

packages/bpk-scrim-utils/src/withScrimmedPortal.tsx

+         * If consumer passes runOnServer as true (meaning they want to render the component on server and client), we render the component as normal once the component has been mounted.
+         * This is to ensure the snapshotted markup (initial render before the component has been mounted) is the same on both server and client.
+         */
+        if (!runOnServer || isPortalReady) {


Would we expect a consumer to be aware of where the component is being rendered (server or browser) and act accordingly to pass a prop in? If so, this is a code smell to me.

Modern web frameworks like Remix have no real concept of "running on server" or "running on browser". The expectation is all components are executed in both environments and it's common practice for components to be required to be self aware of the where they are being rendered, and thus act accordingly.

What's the reason we don't want this to be self aware?

Modern web frameworks like Remix have no real concept of "running on server" or "running on browser". The expectation is all components are executed in both environments

I didn't know that, but I think that's still ok as then you just pass the runOnServer prop which means it runs on both (not just server)? we can make that the default instead (this component is not used anywhere, so we have the freedom of making any change to it).

and it's common practice for components to be required to be self aware of the where they are being rendered, and thus act accordingly.

How can we tell if a component is running on server?
I guess my thoughts here were that consumers can choose whether they want to render on both and use the BpkScrim on server, OR just render it on client.

I didn't know that, but I think that's still ok as then you just pass the runOnServer prop which means it runs on both (not just server)?

Then the naming here should be improved, runOnServer suggests only the server.

But it still raises the question, do we need flexibility here? Why can't this component decide what should happen depending on the environment?

How can we tell if a component is running on server?

common practice is to check for the presence of window, but it depends what the requirements are. If we need the document to be available, we should check for that.

if (typeof window === 'undefined')

totally agree with Chris and probably that's the source of my confusion in the other comment

Then the naming here should be improved, runOnServer suggests only the server.

I thought we always run on either just client, or both client and server so it's clear that this will run on both (although also added a comment above to explain it)

But it still raises the question, do we need flexibility here? Why can't this component decide what should happen depending on the environment?

The component does decide what should happen depending on the env, I just didn't want to force the BpkScrim on the initial render on the client unless required i.e. to match the server initial render also thinking whether maybe in the future we want to render something else on the server, not the BpkScrim

Maybe this comment helps here https://github.com/Skyscanner/backpack/pull/3085/files#r1395443998

Do all our microsites run on both client and server completely?

There's a big mix. Banana is CSR only. Falcon and hotels do SSR. OCs SSR the HTML and deliver it when requested.

Agree with previous comments here, replacing runOnServer prop and it's associated behaviour with if (typeof window === 'undefined') should do the trick 👍

Ah, yes, that'll work too, we don't need the prop

I've removed the prop, although just to clarify - the point of the runOnServer prop was to set the expectation whether the component will run only on client only or if it'll run on both. I think using the window object helps us determine whether at run time it's running on server or on client, but I think achieving the same as the prop (i.e. if the intent is for the component to run on both client and server, or just on client so that for client-only projects we don't need to first render the scrim) makes it confusing as when it's running on client we'd need to check if it's already ran on server 🤔

we'd need to check if it's already ran on server

Can you explain what you mean by this? I don't quite follow

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

packages/bpk-scrim-utils/src/withScrimmedPortal.tsx

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

github-actions · 2023-11-16T14:13:43Z

Visit https://backpack.github.io/storybook-prs/3085 to see this build running in a browser.

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

mungodewar · 2023-11-17T16:15:07Z

packages/bpk-scrim-utils/src/withScrimmedPortal.tsx

        useEffect(() => {
            setIsPortalReady(true);
          }, []);


Given all the discussion regarding double render and the general strategy being taken here, I recommend linking to the react docs: https://react.dev/reference/react-dom/hydrate#handling-different-client-and-server-content and popping some of the context from the comments directly into code.

github-actions · 2023-11-20T16:42:26Z

Visit https://backpack.github.io/storybook-prs/3085 to see this build running in a browser.

github-actions · 2023-11-20T17:07:04Z

Visit https://backpack.github.io/storybook-prs/3085 to see this build running in a browser.

arnaugm · 2023-11-21T10:40:29Z

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

  });

  it('renders the wrapped component inside a portal with renderTarget provided', () => {
-    const WrappedComponent = () => <div>Wrapped Component</div>;
-    const ScrimmedComponent = withScrimmedPortal(WrappedComponent);
+    const DialogContent = () => <div data-testid="dialog-content">Dialog content</div>;


what's the difference between this test and the next one?

hmm, yeah, you're right. Nothing really, I'll remove it.

arnaugm · 2023-11-21T10:40:30Z

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

  });

  it('renders the wrapped component inside a portal with renderTarget provided', () => {
-    const WrappedComponent = () => <div>Wrapped Component</div>;
-    const ScrimmedComponent = withScrimmedPortal(WrappedComponent);
+    const DialogContent = () => <div data-testid="dialog-content">Dialog content</div>;


what's the difference between this test and the next one?

arnaugm · 2023-11-21T10:41:36Z

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx

@@ -17,7 +17,7 @@
 */

 import '@testing-library/jest-dom';
-import { render, within, screen } from '@testing-library/react';
+import { render, within, screen, isInaccessible } from '@testing-library/react';


is isInaccessible used somewhere?

Ohh no, I'd added it, but then removed it
I'll remove this, I thought the linting would do that 😅

runmoore

@anambl and I had a chat on zoom. I'm happy with the implementation that is currently in the PR, so approving on he basis that @arnaugm also gives his approval too :)

github-actions · 2023-11-21T17:00:06Z

Visit https://backpack.github.io/storybook-prs/3085 to see this build running in a browser.

Add support for withScrimmedPortal with SSR

1a29230

anambl added the minor Non breaking change label Nov 15, 2023

anambl marked this pull request as ready for review November 16, 2023 09:18

runmoore reviewed Nov 16, 2023

View reviewed changes

arnaugm reviewed Nov 16, 2023

View reviewed changes

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx Outdated Show resolved Hide resolved

arnaugm reviewed Nov 16, 2023

View reviewed changes

packages/bpk-scrim-utils/src/withScrimmedPortal.tsx Outdated Show resolved Hide resolved

arnaugm reviewed Nov 16, 2023

View reviewed changes

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx Outdated Show resolved Hide resolved

Update tests

1154458

arnaugm reviewed Nov 17, 2023

View reviewed changes

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx Outdated Show resolved Hide resolved

arnaugm reviewed Nov 17, 2023

View reviewed changes

packages/bpk-scrim-utils/src/withScrimmedPortal-test.tsx Outdated Show resolved Hide resolved

mungodewar reviewed Nov 17, 2023

View reviewed changes

Remove runOnServer prop

9bde613

Update tests

9ecdeb2

arnaugm reviewed Nov 21, 2023

View reviewed changes

Update tests and comments

7c86d72

runmoore approved these changes Nov 21, 2023

View reviewed changes

anambl merged commit 2fc97b4 into main Nov 22, 2023
9 checks passed

anambl deleted the ARGG-876 branch November 22, 2023 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARGG-876] Add support for withScrimmedPortal with SSR #3085

[ARGG-876] Add support for withScrimmedPortal with SSR #3085

anambl commented Nov 15, 2023 •

edited

Loading

github-actions bot commented Nov 15, 2023

runmoore Nov 16, 2023

anambl Nov 16, 2023

runmoore Nov 16, 2023

arnaugm Nov 16, 2023

anambl Nov 16, 2023

runmoore Nov 16, 2023

mungodewar Nov 17, 2023

anambl Nov 17, 2023

anambl Nov 20, 2023

runmoore Nov 21, 2023

github-actions bot commented Nov 16, 2023

mungodewar Nov 17, 2023

github-actions bot commented Nov 20, 2023

github-actions bot commented Nov 20, 2023

arnaugm Nov 21, 2023

anambl Nov 21, 2023

arnaugm Nov 21, 2023

arnaugm Nov 21, 2023

anambl Nov 21, 2023

runmoore left a comment

github-actions bot commented Nov 21, 2023

[ARGG-876] Add support for withScrimmedPortal with SSR #3085

[ARGG-876] Add support for withScrimmedPortal with SSR #3085

Conversation

anambl commented Nov 15, 2023 • edited Loading

github-actions bot commented Nov 15, 2023

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

github-actions bot commented Nov 16, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2023

github-actions bot commented Nov 20, 2023

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

runmoore left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2023

anambl commented Nov 15, 2023 •

edited

Loading