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: add playground for SSR reproductions #3601

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

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Jun 30, 2023

Details

This adds an ssr-playground/ as a complement to the existing playground/ that is CSR-only.

You can try it out here.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@divmain divmain requested a review from a team as a code owner June 30, 2023 06:02
"@rollup/plugin-replace": "^5.0.2",
"express": "^4.18.2",
"html-entities": "^2.3.3",
"lwc": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be exact versions instead of ^ versions so that Lerna can link them up correctly, and so we know exactly what version someone has when they file repros. This is how we do it in the other playground:

"@lwc/rollup-plugin": "3.0.0",

<script>
window.lwcRuntimeFlags = window.lwcRuntimeFlags || {};
window.lwcRuntimeFlags.ENABLE_LIGHT_DOM_COMPONENTS = true;
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

This script isn't needed, light DOM is GA now.

async function buildResponse(props) {
globalThis.lwc = engineServer;

const cmp = (await import(`${compiledComponentPath}?cacheBust=${Date.now()}`)).default;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the goal here is to bust the cache (presumably during watch mode?), but this does introduce a memory leak. Unfortunately I'm not aware of any good workaround for this in ESM mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave a comment here to that effect. I considered moving back to CommonJS but that broke other things and would be misaligned with the other playground.

"type": "module",
"description": "Playground project to experiment with LWC.",
"scripts": {
"dev": "node ./server.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Without a "build" script, NX doesn't actually build this project. Should it have a prod-mode buildlike the otherplayground`?

"scripts": {
"dev": "node ./server.js",
"csr-dev": "rollup -c --watch",
"ssr:playground": "cd .. && yarn run build && cd ssr-playground && yarn run dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

These two scripts don't do anything in StackBlitz, right? Do we need them?

import Parent from "x/parent";

const elm = createElement("x-parent", { is: Parent });
elm.tabIndex = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why set the tabindex here? Also typically a11y audits will warn about 1) tabindex greater than 0, and 2) tabindex without aria-label.

Comment on lines +109 to +120
if (event.code === 'ERROR') {
// eslint-disable-next-line no-console
console.error(event.error);
}
if (event.code === 'START') {
// eslint-disable-next-line no-console
process.stdout.write('Compiling...');
}
if (event.code === 'END') {
// eslint-disable-next-line no-console
console.log(' done!');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These could be else if or switches to improve readability a bit,

input: 'src/entry-client-ssr.js',
output: {
file: 'dist/entry-rehydrate.js',
format: 'cjs'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is written as cjs but consumed in a <script>. Can we use esm and <script type="module">?

return res.sendFile(path.resolve(__dirname, './static/csr.html'));
});

app.get('*', (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
app.get('*', (req, res) => {
app.get('/', (req, res) => {

* accepts localhost:3000/foobar; I think we should probably be stricter.

import lwc from '@lwc/rollup-plugin';
import replace from '@rollup/plugin-replace';
import alias from '@rollup/plugin-alias';
import simpleRollupConfig from'./rollup.config.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove csr-dev then we can potentially move this into this file, and have one single rollup.config.js file.

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.

None yet

2 participants