-
Notifications
You must be signed in to change notification settings - Fork 926
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
[labs/observers] Prevent failing in SSR #4591
[labs/observers] Prevent failing in SSR #4591
Conversation
🦋 Changeset detectedLatest commit: 13e9db7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
📊 Tachometer Benchmark ResultsSummary⏳ Benchmarks are currently running. Results below are out of date. nop-update
render
update
update-reflect
Results⏳ Benchmarks are currently running. Results below are out of date. this-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
14f8bef
to
2ab0099
Compare
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.
Looks good to me. @sorvell wdyt?
The size of lit-html.js and lit-core.min.js are as expected. |
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.
There's an important unresolved comment now
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.
Thanks!
What is left here guys? |
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.
Thanks!
This PR makes observer controllers not to initialize the observers when the constructor is called in SSR environment, preventing a crash in SSR environment. Fixes #4590.
The
isServer
check being beforewindow.xObserver
check is intentional aswindow
won't exist in Node. Also there is no need to warn the developer in SSR as opposed to when we are in the browser but the observer is not supported.There is clean-up potential after #3386 has been merged, namely adding an abstract
createObserver()
method to the base class for all common browser observers and moving theisServer
check to the base class and conditionally calling it.