-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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(notification-center): fix for ssr issues #2395
Conversation
@@ -77,6 +77,7 @@ | |||
"axios": "^0.26.1", | |||
"chroma-js": "^2.4.2", | |||
"date-fns": "^2.29.1", | |||
"lodash.clonedeep": "^4.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lodash.clonedeep
is used instead of structuredClone
which is only available rn in the browser (web API)
@@ -17,6 +17,7 @@ export default [ | |||
file: packageJson.main, | |||
format: 'cjs', | |||
sourcemap: true, | |||
interop: 'auto', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue was with the @emotion/styled
which exports the styled.div
functions in the default export, which works in the ESM module, but not in CJS which natively doesn't support it well.
The option from the docs:
Controls how Rollup handles default, namespace and dynamic imports from external dependencies in formats like CommonJS that do not natively support these concepts.
"auto" combines both "esModule" and "default" by injecting helpers that contain code that detects at runtime if the required value contains the [__esModule property](https://rollupjs.org/guide/en/#outputesmodule).
if (isSSR) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are in the react to web component proxy file
when it's used during SSR just do nothing, because the Web APIs do not exist on server side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌟
What change does this PR introduce?
It fixes a couple of issues related to running the
notification-center
package in the SSR app.The Discord thread: https://discord.com/channels/895029566685462578/1057740995321663689
Why was this change needed?
To allow SSR-rendered apps use the
notification-center
package.Other information (Screenshots)