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: implement wire + context for SSR compiler #4807

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

divmain
Copy link
Contributor

@divmain divmain commented Nov 8, 2024

Details

This implements both the normal & contextful varieties of @wire for the new SSR compiler. The wire and context-* fixture tests are now passing (down to 39 total failures).

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-14631064

@divmain divmain requested a review from a team as a code owner November 8, 2024 07:04
export default class Component extends LightningElement {
@wire(WireAdapter) foo;
export default class ConsumerComponent extends LightningElement {
@wire(WireAdapter, { unused: '$definitelyUnused' }) foo;
Copy link
Contributor Author

@divmain divmain Nov 8, 2024

Choose a reason for hiding this comment

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

Wire adapters with config deserve more test coverage. I think I implemented support for wire-config correctly but, because my time is constrained, I did not spend time thinking through the scenarios here and adding tests. Should probably be addressed in a follow-up WI.

@@ -44,6 +44,7 @@
}
},
"dependencies": {
"@babel/helper-validator-identifier": "^7.25.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulling this in seemed like a reasonable thing to do. In @wire config, we have to parse '$stringLiteralReferencesWithDollar' from the config object that is passed in (see previous comment about $definitelyUnused). Detecting/validating identifier names is more complex than you'd think, and outsourcing this to a purpose-built Babel utility seemed sane to me. We can reimplement internally, if y'all prefer.

@@ -27,7 +29,7 @@ type RenderCallExpression = SimpleCallExpression & {
};

const bGenerateMarkup = esTemplate`
export async function* generateMarkup(tagName, props, attrs, slotted) {
export async function* generateMarkup(tagName, props, attrs, slotted, parent) {
Copy link
Contributor Author

@divmain divmain Nov 8, 2024

Choose a reason for hiding this comment

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

You'll see these new parent and instance arguments being passed around a few places. It doesn't need to penetrate into the call stack very far, just far enough to reach the __establishContextfulRelationship invocations a few lines below.

let connectWireAdapterCode: Statement[] = [];
if (state.wireAdapters.length) {
connectWireAdapterCode = bWireAdaptersPlumbing(state.wireAdapters);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no wire adapters are detected on the component, we don't bother injecting the wire-related code.

// inadvertently mutating things like Wire adapters.
if (!state.isLWC) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lost a few minutes trying to figure out what was going wrong here. Pretty sure this is the right thing to do in this case, and you might consider doing similar checks in other places where the AST is being mutated.

@@ -32,7 +32,7 @@ const bYieldEscapedString = esTemplateWithYield`
yield String(${0});
break;
default:
yield ${0} ? htmlEscape(${0}.toString()) : '\\u200D';
yield ${0} ? htmlEscape(${0}.toString()) : ${2} ? '\\u200D' : '';
Copy link
Contributor Author

@divmain divmain Nov 8, 2024

Choose a reason for hiding this comment

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

This was not strictly related to the @wire stuff, but was necessary in order to get the context-no-provider test to pass, where this happens in the template: <div>I have no context for this{foo}</div>.

Prior to my change here, a \u200D character would be inserted following the word "this" in the rendered markup. That's not what we want - the zero-width character should only be rendered if there are no adjacent text nodes. This check already happens in case 'string' a few lines above and was duplicated here.

@@ -44,6 +44,7 @@
}
},
"devDependencies": {
"@lwc/shared": "8.7.0"
"@lwc/shared": "8.7.0",
"@lwc/engine-core": "8.7.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the types are used, nothing from @lwc/engine-core is used at runtime.

childLe: LightningElement
): void {
contextfulRelationships.set(childLe, parentLe);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be an optimization opportunity. Creating & preserving an actual stack of components is probably a worse solution, as it would 1) be slow, and 2) require passing that stack object through many intermediate functions.

However, I could see doing something like childLe[SECRET_PARENT_SYMBOL] = parentLe or something like that rather than using the WeakMap. I'm not sure which is faster; I just picked one and went with it.

return;
}
const contextfulStack = getContextfulStack(contextConsumer);
for (const ancestor of contextfulStack) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This for loop and the getContextfulStack call could probably be turned in to a while (ancestor = getAncestor(ancestor)) loop or something like that. The existing implementation is probably slower than the alternative, but it is also easier to read. Optimization opportunity if this turns out to be a hot path, unlikely though that may be.

consumerConnectedCallback(consumer)
);
};
}
Copy link
Contributor Author

@divmain divmain Nov 8, 2024

Choose a reason for hiding this comment

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

The implementation in this module is... seemingly indirect. Unfortunately, I believe that complexity to be necessary because we must match the existing observable behavior & mechanics of how wire adapters work today. If we were reinventing the wire protocol, this could probably be vastly simplified. As it is, this is probably the best we can do.

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.

1 participant