-
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
Changes from 4 commits
cfb5f7d
719a6e9
a807713
bd16f3e
53bc835
3efd32a
dce0888
7385cc2
af1465d
f4e6b89
24fba78
32af684
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"entry": "x/main", | ||
"ssrFiles": { | ||
"error": "error-ssr.txt", | ||
"expected": "expected-ssr.html" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe 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 commentThe 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). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<fixture-test> | ||
<template shadowrootmode="open"> | ||
<div> | ||
context one value: context one | ||
</div> | ||
<div> | ||
context two value: context two | ||
</div> | ||
</template> | ||
</fixture-test> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
class MockContextSignal { | ||
connectProvidedComponent; | ||
disconnectProvidedComponent; | ||
providedContextSignal; | ||
|
||
constructor(initialValue, contextDefinition, fromContext) { | ||
this.value = initialValue; | ||
this.contextDefinition = contextDefinition; | ||
this.fromContext = fromContext; | ||
trustedContext.add(this); | ||
} | ||
[connectContext](runtimeAdapter) { | ||
this.connectProvidedComponent = runtimeAdapter.component; | ||
|
||
runtimeAdapter.provideContext(this.contextDefinition, this); | ||
|
||
if (this.fromContext) { | ||
runtimeAdapter.consumeContext(this.fromContext, (providedContextSignal) => { | ||
this.providedContextSignal = providedContextSignal; | ||
this.value = providedContextSignal.value; | ||
}); | ||
} | ||
} | ||
[disconnectContext](component) { | ||
this.disconnectProvidedComponent = component; | ||
} | ||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Wait, it provides itself as a param? Trippy... |
||
return contextDefinition; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
<template> | ||
<div>context one value: {contextOne.value}</div> | ||
<div>context two value: {contextTwo.value}</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { LightningElement } from 'lwc'; | ||
import { defineContext } from 'x/contextManager'; | ||
const contextFactory = defineContext(); | ||
export default class Main extends LightningElement { | ||
contextOne = contextFactory('context one'); | ||
contextTwo = contextFactory('context two'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"entry": "x/main", | ||
"ssrFiles": { | ||
"error": "error-ssr.txt", | ||
"expected": "expected-ssr.html" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Attempted to connect to trusted context but received the following error: le[contextfulKeys[i]][connectContext2] is not a function |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<fixture-test> | ||
<template shadowrootmode="open"> | ||
<div> | ||
context value: | ||
</div> | ||
</template> | ||
</fixture-test> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// This is a malformed context signal that does not implement the connectContext or disconnectContext methods | ||
class MockMalformedContextSignal { | ||
constructor() { | ||
trustedContext.add(this); | ||
} | ||
} | ||
|
||
export const defineMalformedContext = () => { | ||
return () => new MockMalformedContextSignal(); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
<div>context value: {malformedContext.value}</div> | ||
</template> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
import { defineMalformedContext } from 'x/contextManager'; | ||
export default class Root extends LightningElement { | ||
malformedContext = defineMalformedContext()(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. It was redundant (mistake from the past) |
||
if (requiredFeatureFlags) { | ||
requiredFeatureFlags.forEach((featureFlag) => { | ||
lwcSsr.setFeatureFlagForTest(featureFlag, false); | ||
}); | ||
} | ||
} | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for leaving this comment. |
||
|
||
connectedCallback() { | ||
this.showTree = true; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,12 +6,17 @@ | |
*/ | ||
|
||
import path from 'node:path'; | ||
import { vi, describe } from 'vitest'; | ||
import { vi, describe, beforeAll, afterAll } from 'vitest'; | ||
import { rollup } from 'rollup'; | ||
import lwcRollupPlugin from '@lwc/rollup-plugin'; | ||
import { testFixtureDir, formatHTML, pluginVirtual } from '@lwc/test-utils-lwc-internals'; | ||
import { serverSideRenderComponent } from '@lwc/ssr-runtime'; | ||
import { DEFAULT_SSR_MODE, type CompilationMode } from '@lwc/shared'; | ||
import { serverSideRenderComponent, setFeatureFlagForTest } from '@lwc/ssr-runtime'; | ||
import { | ||
DEFAULT_SSR_MODE, | ||
type CompilationMode, | ||
setContextKeys, | ||
setTrustedContextSet, | ||
} from '@lwc/shared'; | ||
import { expectedFailures } from './utils/expected-failures'; | ||
import type { LightningElementConstructor } from '@lwc/ssr-runtime'; | ||
|
||
|
@@ -92,6 +97,21 @@ async function compileFixture({ entry, dirname }: { entry: string; dirname: stri | |
} | ||
|
||
describe.concurrent('fixtures', () => { | ||
beforeAll(() => { | ||
// Defining context keys and trusted context must be done once before related tests are ran. | ||
const connectContext = Symbol('connectContext'); | ||
const disconnectContext = Symbol('disconnectContext'); | ||
const trustedContext = new WeakSet(); | ||
setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', true); | ||
setContextKeys({ connectContext, disconnectContext }); | ||
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 commentThe 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 |
||
}); | ||
afterAll(() => { | ||
setFeatureFlagForTest('ENABLE_EXPERIMENTAL_SIGNALS', false); | ||
}); | ||
testFixtureDir<FixtureConfig>( | ||
{ | ||
root: path.resolve(__dirname, '../../../engine-server/src/__tests__/fixtures'), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
/* | ||
* Copyright (c) 2024, salesforce.com, inc. | ||
* All rights reserved. | ||
* SPDX-License-Identifier: MIT | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT | ||
*/ | ||
import { | ||
type ContextProvidedCallback, | ||
type ContextBinding as IContextBinding, | ||
isTrustedContext, | ||
getContextKeys, | ||
isUndefined, | ||
keys, | ||
ArrayFilter, | ||
} from '@lwc/shared'; | ||
import { getContextfulStack } from './wire'; | ||
import { type LightningElement, SYMBOL__CONTEXT_VARIETIES } from './lightning-element'; | ||
import type { Signal } from '@lwc/signals'; | ||
|
||
class ContextBinding<C extends LightningElement> implements IContextBinding<LightningElement> { | ||
component: C; | ||
|
||
constructor(component: C) { | ||
this.component = component; | ||
} | ||
|
||
provideContext<V extends object>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
contextVariety: V, | ||
providedContextSignal: Signal<unknown> | ||
): void { | ||
const contextVarieties = this.component[SYMBOL__CONTEXT_VARIETIES]; | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is a nice ergonomic touch; thank you. |
||
return; | ||
} | ||
contextVarieties.set(contextVariety, providedContextSignal); | ||
} | ||
|
||
consumeContext<V extends object>( | ||
contextVariety: V, | ||
contextProvidedCallback: ContextProvidedCallback | ||
): void { | ||
const contextfulStack = getContextfulStack(this.component); | ||
for (const ancestor of contextfulStack) { | ||
// If the ancestor has the specified context variety, consume it and stop searching | ||
const ancestorContextVarieties = ancestor[SYMBOL__CONTEXT_VARIETIES]; | ||
if (ancestorContextVarieties.has(contextVariety)) { | ||
contextProvidedCallback(ancestorContextVarieties.get(contextVariety)); | ||
break; | ||
} | ||
} | ||
} | ||
} | ||
|
||
export function connectContext(le: LightningElement) { | ||
const contextKeys = getContextKeys(); | ||
|
||
if (isUndefined(contextKeys)) { | ||
return; | ||
} | ||
|
||
const { connectContext } = contextKeys; | ||
|
||
const enumerableKeys = keys(le); | ||
const contextfulKeys = ArrayFilter.call(enumerableKeys, (enumerableKey) => | ||
isTrustedContext((le as any)[enumerableKey]) | ||
); | ||
|
||
if (contextfulKeys.length === 0) { | ||
return; | ||
} | ||
|
||
try { | ||
for (let i = 0; i < contextfulKeys.length; i++) { | ||
(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 commentThe 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 commentThe 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. |
||
throw new Error( | ||
`Attempted to connect to trusted context but received the following error: ${ | ||
err.message | ||
}` | ||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,13 @@ export { | |
sanitizeHtmlContent, | ||
normalizeClass, | ||
normalizeTabIndex, | ||
setContextKeys, | ||
setTrustedSignalSet, | ||
setTrustedContextSet, | ||
} from '@lwc/shared'; | ||
|
||
export { setFeatureFlag, setFeatureFlagForTest } from '@lwc/features'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add |
||
|
||
export { ClassList } from './class-list'; | ||
export { | ||
LightningElement, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,13 +29,16 @@ import { ClassList } from './class-list'; | |
import { mutationTracker } from './mutation-tracker'; | ||
import { descriptors as reflectionDescriptors } from './reflection'; | ||
import { getReadOnlyProxy } from './get-read-only-proxy'; | ||
import { connectContext } from './context'; | ||
import type { Attributes, Properties } from './types'; | ||
import type { Stylesheets } from '@lwc/shared'; | ||
import type { Signal } from '@lwc/signals'; | ||
|
||
type EventListenerOrEventListenerObject = unknown; | ||
type AddEventListenerOptions = unknown; | ||
type EventListenerOptions = unknown; | ||
type ShadowRoot = unknown; | ||
type ContextVarieties = Map<unknown, Signal<unknown>>; | ||
|
||
export type LightningElementConstructor = typeof LightningElement; | ||
|
||
|
@@ -46,6 +49,7 @@ interface PropsAvailableAtConstruction { | |
export const SYMBOL__SET_INTERNALS = Symbol('set-internals'); | ||
export const SYMBOL__GENERATE_MARKUP = Symbol('generate-markup'); | ||
export const SYMBOL__DEFAULT_TEMPLATE = Symbol('default-template'); | ||
export const SYMBOL__CONTEXT_VARIETIES = Symbol('context-varieties'); | ||
|
||
export class LightningElement implements PropsAvailableAtConstruction { | ||
static renderMode?: 'light' | 'shadow'; | ||
|
@@ -73,6 +77,7 @@ export class LightningElement implements PropsAvailableAtConstruction { | |
#props!: Properties; | ||
#attrs!: Attributes; | ||
#classList: ClassList | null = null; | ||
[SYMBOL__CONTEXT_VARIETIES]: ContextVarieties = new Map(); | ||
|
||
constructor(propsAvailableAtConstruction: PropsAvailableAtConstruction & Properties) { | ||
assign(this, propsAvailableAtConstruction); | ||
|
@@ -82,6 +87,11 @@ export class LightningElement implements PropsAvailableAtConstruction { | |
this.#props = props; | ||
this.#attrs = attrs; | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This is wonderfully straightforward :) |
||
|
||
// Class should be set explicitly to avoid it being overridden by connectedCallback classList mutation. | ||
if (attrs.class) { | ||
this.className = attrs.class; | ||
|
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.
Uh oh!
There was an error while loading. Please reload this page.
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.