-
Notifications
You must be signed in to change notification settings - Fork 414
@W-17388345 - add context support to ssrv2 component lifecycle #5387
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
Conversation
(global as any).trustedContext = trustedContext; | ||
(global as any).connectContext = connectContext; | ||
(global as any).disconnectContext = disconnectContext; |
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.
Gross. Can we at least call them __testTrustedContext
or something, so it's more clear elsewhere what they're for?
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.
Can these be setup as a singleton and then imported from the spec files? Seems like this works just fine, so that's not a problem. But importing a singleton would be less surprising in the future when we forget what this stuff is, and compared to seeing a reference and trying to remember what it is and how it works.
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.
I don't love the globals either. In theory we could do imports but is there precedence for this? If we introduce logging (align V1/V2 error handling so they both log instead of V2 throwing) then we could do away with these fixtures and have cleaner hydration tests. Using a different naming complicates things as they are symbols. I'll bring this up next parking lot and fix this in a follow up depending on what we decide if that is ok.
} from '@lwc/shared'; | ||
|
||
export { setFeatureFlag, setFeatureFlagForTest } from '@lwc/features'; |
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.
Add @lwc/features
as a dev dependency so that it gets properly bundled.
(le as any)[contextfulKeys[i]][connectContext](new ContextBinding(le)); | ||
} | ||
} catch (err: any) { | ||
if (process.env.NODE_ENV !== 'production') { |
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.
Why do we ignore errors in production?
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.
It is similar to the above where we conditionally show an error in dev/test. But because this is at the end of a function, we don't need an early-return the way we do in the other instance.
@@ -0,0 +1 @@ | |||
Attempted to connect to trusted context but received the following error: Multiple contexts of the same variety were provided. |
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.
Why is SSR v2 an error, but engine-server not?
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 don't have the concept of logging in SSRv2 runtime (from what I can tell - feel free to correct me). I didn't feel it necessary to introduce that now, simply wrapping in !production felt adequate. However if either/both of you disagree, i'd be happy to introduce logging (not throwing) in a follow up PR (it will reduce the amount of testing needed).
|
||
export const defineContext = (fromContext) => { | ||
const contextDefinition = (initialValue) => | ||
new MockContextSignal(initialValue, contextDefinition, fromContext); |
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.
Wait, it provides itself as a param? Trippy...
@@ -279,12 +279,6 @@ function createHCONFIG2JSPreprocessor(config, logger, emitter) { | |||
const location = path.relative(basePath, filePath); | |||
log.error('Error processing “%s”\n\n%s\n', location, error.stack || error.message); | |||
done(error, null); | |||
} finally { |
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.
Why is this block deleted?
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.
It was redundant (mistake from the past)
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.
Approval, modulo a couple of minor changes. No need for a re-review if you address the feedback and GitHub still gives you the green button. If you need another approval after making changes, please ping me.
(global as any).trustedContext = trustedContext; | ||
(global as any).connectContext = connectContext; | ||
(global as any).disconnectContext = disconnectContext; |
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.
Can these be setup as a singleton and then imported from the spec files? Seems like this works just fine, so that's not a problem. But importing a singleton would be less surprising in the future when we forget what this stuff is, and compared to seeing a reference and trying to remember what it is and how it works.
@@ -2,7 +2,8 @@ import { LightningElement, api } from 'lwc'; | |||
import { defineMalformedContext } from 'x/contextManager'; | |||
export default class Root extends LightningElement { | |||
@api showTree = false; | |||
malformedContext = defineMalformedContext()(); | |||
// Only test in CSR right now as SSR throws which prevents content from being rendered. There is additional fixtures ssr coverage for this case. | |||
malformedContext = typeof window !== 'undefined' ? defineMalformedContext()() : undefined; |
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 for leaving this comment.
setTrustedContextSet(trustedContext); | ||
(global as any).trustedContext = trustedContext; | ||
(global as any).connectContext = connectContext; | ||
(global as any).disconnectContext = disconnectContext; |
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.
Oh I see you're overwriting these global objects here. This gets extra confusing. If the globals were moved into a separate module as a singleton, and if that module also exported setTrustedContext
and similar functions, that would be preferred. Unless there's a reason this must be setup using globals, in which case let's find a place to document the reasoning.
if (contextVarieties.has(contextVariety)) { | ||
if (process.env.NODE_ENV !== 'production') { | ||
throw new Error('Multiple contexts of the same variety were provided.'); | ||
} |
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.
This is a nice ergonomic touch; thank you.
this.component = component; | ||
} | ||
|
||
provideContext<V extends object>( |
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.
I know these methods are documented elsewhere, perhaps in the CSR implementation, but it'd be nice to have doc strings for these methods here as well.
(le as any)[contextfulKeys[i]][connectContext](new ContextBinding(le)); | ||
} | ||
} catch (err: any) { | ||
if (process.env.NODE_ENV !== 'production') { |
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.
It is similar to the above where we conditionally show an error in dev/test. But because this is at the end of a function, we don't need an early-return the way we do in the other instance.
if (lwcRuntimeFlags.ENABLE_EXPERIMENTAL_SIGNALS) { | ||
// Setup context before connected callback is executed | ||
connectContext(this); | ||
} |
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.
This is wonderfully straightforward :)
@divmain @wjhsf thank you both for your reviews - unfortunately there were other issues with the fixtures test in addition to the ugly global assignments. Due to disparities in dev/production runs of vitest and how the thrown Errors are handled the CI cannot pass. I've added some basic unit coverage and removed the fixtures coverage for now. I created a separate work item to review the coverage and make sure it's adequate: W-18783313 |
Details
Add SSRv2 context support to the component lifecycle. It has already been added for SSRv2 / CSR so now state-managers can function without additional shims and extending non-lightning components.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-17388345