Skip to content
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

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Nov 14, 2022

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?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

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

@divmain divmain requested a review from lpomerleau November 14, 2022 06:26
Copy link
Collaborator

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

@divmain divmain closed this Nov 14, 2022
@divmain divmain reopened this Nov 14, 2022
packages/@lwc/engine-server/src/types.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-server/src/renderer.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-server/src/types.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-server/src/types.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-dom/src/renderer/context.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/context.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/context.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts Outdated Show resolved Hide resolved
@divmain divmain force-pushed the dbustad/context-engine-server branch 4 times, most recently from 7f16801 to f7814d9 Compare February 17, 2023 07:36
yarn.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@nolanlawson nolanlawson left a 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

@divmain divmain force-pushed the dbustad/context-engine-server branch 3 times, most recently from f76640c to dd28ba7 Compare February 17, 2023 23:37
@divmain divmain force-pushed the dbustad/context-engine-server branch from 023c9f5 to 101e238 Compare February 18, 2023 00:03
@divmain divmain force-pushed the dbustad/context-engine-server branch from ae31fe9 to e4686d5 Compare February 21, 2023 20:30
@divmain divmain force-pushed the dbustad/context-engine-server branch from e4686d5 to 1cd0c77 Compare February 21, 2023 20:46
@abdulsattar
Copy link
Contributor

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>

@divmain divmain merged commit 5aad78e into master Feb 22, 2023
@divmain divmain deleted the dbustad/context-engine-server branch February 22, 2023 23:42
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.

6 participants