@W-17388345 - add context support to ssrv2 component lifecycle#5387
@W-17388345 - add context support to ssrv2 component lifecycle#5387jhefferman-sfdc merged 12 commits intomasterfrom
Conversation
| (global as any).trustedContext = trustedContext; | ||
| (global as any).connectContext = connectContext; | ||
| (global as any).disconnectContext = disconnectContext; |
There was a problem hiding this comment.
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.
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.
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.
| setTrustedContextSet, | ||
| } from '@lwc/shared'; | ||
|
|
||
| export { setFeatureFlag, setFeatureFlagForTest } from '@lwc/features'; |
There was a problem hiding this comment.
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.
Why do we ignore errors in production?
There was a problem hiding this comment.
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. No newline at end of file | |||
There was a problem hiding this comment.
Why is SSR v2 an error, but engine-server not?
There was a problem hiding this comment.
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.
Wait, it provides itself as a param? Trippy...
| 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.
It was redundant (mistake from the past)
divmain
left a comment
There was a problem hiding this comment.
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.
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.
| @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.
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.
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.
This is a nice ergonomic touch; thank you.
| this.component = component; | ||
| } | ||
|
|
||
| provideContext<V extends object>( |
There was a problem hiding this comment.
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.
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.
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