Skip to content

@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

Merged
merged 12 commits into from
Jun 14, 2025

Conversation

jhefferman-sfdc
Copy link
Collaborator

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?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

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

GUS work item

W-17388345

@jhefferman-sfdc jhefferman-sfdc requested a review from a team as a code owner June 13, 2025 14:56
Comment on lines 190 to 192
(global as any).trustedContext = trustedContext;
(global as any).connectContext = connectContext;
(global as any).disconnectContext = disconnectContext;
Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

@jhefferman-sfdc jhefferman-sfdc Jun 13, 2025

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';
Copy link
Collaborator

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') {
Copy link
Collaborator

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?

Copy link
Collaborator

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.
Copy link
Collaborator

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?

Copy link
Collaborator Author

@jhefferman-sfdc jhefferman-sfdc Jun 13, 2025

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);
Copy link
Collaborator

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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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)

Copy link
Collaborator

@divmain divmain left a 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.

Comment on lines 190 to 192
(global as any).trustedContext = trustedContext;
(global as any).connectContext = connectContext;
(global as any).disconnectContext = disconnectContext;
Copy link
Collaborator

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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.');
}
Copy link
Collaborator

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>(
Copy link
Collaborator

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') {
Copy link
Collaborator

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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wonderfully straightforward :)

@jhefferman-sfdc
Copy link
Collaborator Author

jhefferman-sfdc commented Jun 14, 2025

@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

@jhefferman-sfdc jhefferman-sfdc enabled auto-merge (squash) June 14, 2025 04:16
@jhefferman-sfdc jhefferman-sfdc merged commit 446e23b into master Jun 14, 2025
6 checks passed
@jhefferman-sfdc jhefferman-sfdc deleted the jhefferman/context-ssrv2 branch June 14, 2025 04:23
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.

3 participants