-
Notifications
You must be signed in to change notification settings - Fork 401
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
feat(engine-server): contextful wires #3165
Conversation
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.
Looks like karma tests are failing? Error: Unable to find associated VM for [object HTMLDivElement]
This LGTM, but I'm really unfamiliar with the context/wire code so I'd prefer to defer to @jodarove or another subject matter expert here.
packages/@lwc/engine-server/src/__tests__/fixtures/context-nested/wire-adapter.js
Outdated
Show resolved
Hide resolved
7f16801
to
f7814d9
Compare
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.
A few questions about tests and coverage, but otherwise LGTM
f76640c
to
dd28ba7
Compare
023c9f5
to
101e238
Compare
packages/@lwc/engine-server/src/__tests__/fixtures/context-no-provider/expected.html
Show resolved
Hide resolved
ae31fe9
to
e4686d5
Compare
e4686d5
to
1cd0c77
Compare
LGTM. A minor nitpick: can we add a testcase for ensuring data is passed into slotted components? // x/test.html
<template>
<x-provider>
<x-consumer></x-consumer>
</x-provider>
</template> |
Details
We have not landed on a long-term strategy for context in LWC. However, we did agree that, since contextful wires work in the browser, they should also be supported in the server. This PR adds that functionality.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?