Skip to content

Conversation

@MarcusNotheis
Copy link
Collaborator

@MarcusNotheis MarcusNotheis commented Feb 5, 2023

The UI5 Web Components for React project is currently looking in how we can support Server Side Rendering with minimal efforts. As we're using some of your base package API's, we need to make sure that they don't throw errors when being used in server side runtimes.

The Device used to execute various functions on client only API when imported which caused errors when used in SSR frameworks like NextJS.
With this PR all internal calls are refactored to use SSR-safe getters and all detections return false on the server. The behaviour on the client and the public API is unchanged.

In addition to that, the Boot, InitialConfiguration and getSharedResource and CustomStyle are now SSR safe as well.

Part of #2240

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Unless anyone else wants to review today, I can +1

@pskelin
Copy link
Contributor

pskelin commented Feb 8, 2023

can we add a test? this will break pretty easily and no one will notice.

I was thinking a very simple nodejs import that should pass without an error, like:
node -e 'import("./dist/Button.js")'

this can be in a package.json script and called in addition to the current test command.

For this to work, I needed to add "type": "module" to all package.json files, but we don't ship CJS so it should be safe to add.

Additionally, I get an error in your branch with the above test:

file:///Users/i030191/work/ui5-webcomponents/packages/base/dist/StaticArea.js:2
if (!customElements.get("ui5-static-area")) {
^

ReferenceError: customElements is not defined

@MarcusNotheis
Copy link
Collaborator Author

can we add a test? this will break pretty easily and no one will notice.

I was thinking a very simple nodejs import that should pass without an error, like: node -e 'import("./dist/Button.js")'

I think until we can use such a test it will take quite some time as the webcomponents themself can't render on the server yet. As this PR is mainly about the changes in the Device I can try to add a test for the device.

@pskelin
Copy link
Contributor

pskelin commented Feb 8, 2023

Fine with me, the device import already works.

➜   ✗ node -e 'import("./dist/Device.js")'
➜   ✗ echo $?
0

I was not talking about the rendering of the components, but their imports should not fail on the server. I think you import the components as well, even just to render their tags via the wrappers.

Anyway, it's fine to add such an import test to the components with another change, let's do a base test for now.

@MarcusNotheis
Copy link
Collaborator Author

MarcusNotheis commented Feb 8, 2023

Just had a call with @pskelin:
I've written some initial test but it is not executed as part of the default test pipeline.
The blocker is the mixture of ESM and CJS in the project which is not a low hanging fruit to solve - we've decided to put the tests for SSR on the backlog.

Current Challenges:

  • we can't require the Device because it is using ESM
  • we can add type: module to the package.json, but then the wdio and nps scripts have errors because they are using CJS.

@pskelin pskelin merged commit 2830c67 into main Feb 8, 2023
@pskelin pskelin deleted the refactor/device-internal-getters branch February 8, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants