feat: bake context into lwc framework #4995
Conversation
rax-it
changed the title
rax-it
changed the title
rax-it
changed the title
There was a problem hiding this comment.
This seems like a lot of code just for a Karma test. What is the downside of importing @lwc/state directly? It's already OSS.
| await new Promise((resolve) => requestAnimationFrame(resolve)); | ||
| expect(state.subscribers.size).toBe(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Personally I'd prefer to put all the context-related tests together in one folder, but it's not a big deal.
There was a problem hiding this comment.
If we're going to inline @lwc/state twice, I think we should at least hoist this up to the Karma level so that we can just import it from both. We could do the same thing we're doing with test-utis to make it importable.
| detail: { ...detail, key: getContextKeys().contextEventKey }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Have you looked at the Web Components Community Group proposal for a "context" event? This is an attempt to standardize the concept of passing context between ancestor/descendant components across web component frameworks.
The advantage of using this protocol is that, hypothetically, a non-LWC framework like Lit could use the same protocol and be interoperable with LWC in the same DOM tree.
| let isProvidingContext = false; | ||
| const providedContextVarieties = new Map<unknown, Signal<unknown>>(); | ||
| const contextRuntimeAdapter = { | ||
| isServerSide: false, |
There was a problem hiding this comment.
engine-core may execute in a server context, in which case process.env.IS_BROWSER will be false. Does that help here?
|
|
||
| component.dispatchEvent(event); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This may perform better in JS engines if it's an explicit class that we are newing.
| if (!isProvidingContext) { | ||
| isProvidingContext = true; | ||
|
|
||
| component.addEventListener(ContextEventName, (event: any) => { |
There was a problem hiding this comment.
We typically use the renderer API for this. Although it's unclear to me how this would work in an SSR context since we don't implement an event listener shim. For the ContextProvider API we implemented a full SSR-compatible shim to make it work (#3165); presumably we ought to do the same here if this needs to run server-side.
| logError( | ||
| 'Multiple contexts of the same variety were provided. Only the first context will be used.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
This code is not covered in the Karma tests. You can download the coverage report and see that actually quite a bit is uncovered.
There was a problem hiding this comment.
I feel that this code has a large amount of overlap with the existing ContextProvider implementation. Besides (probably?) needing an SSR implementation, it seems to me that it may make sense to implement the two on a shared protocol rather than having two completely distinct side-by-side implementations (unless we truly believe we can deprecate ContextProvider.) 🙂
|
@nolanlawson I would love to see @divmain opinion on some of the stuff you mentioned as he has been a driving force behind this, so putting this on hold until then. |
|
@rax-it I've scheduled a DevSync in January. Let's all 3 of us go over the design then! |
Details
Moving ContextfulLightningElement shim to LWC engine core
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item
W-14777911