-
Notifications
You must be signed in to change notification settings - Fork 397
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(ssr): add support for superclasses @ W-17178263 #4903
Conversation
it's too late for descriptive commits
@@ -65,11 +66,14 @@ const bGenerateMarkup = esTemplate` | |||
yield* tmplFn(props, attrs, slotted, ${/*component class*/ 3}, instance); | |||
yield \`</\${tagName}>\`; | |||
} | |||
`<ExportNamedDeclaration>; | |||
${/* component class */ 3}[__SYMBOL__GENERATE_MARKUP] = generateMarkup; |
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.
This was previously a separate builder, but it doesn't actually need to be.
const bExposeTemplate = esTemplate` | ||
if (${/*template*/ is.identifier}) { | ||
${/* component class */ is.identifier}[__SYMBOL__DEFAULT_TEMPLATE] = ${/*template*/ 0} | ||
} | ||
`<IfStatement>; |
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 tried using isNullableOf
so that this could be part of bGenerateMarkup
, but it didn't work, so I just kept it separate.
@@ -97,7 +97,7 @@ const getReplacementNode = ( | |||
validateReplacement.name || | |||
'(could not determine)'; | |||
const actualType = Array.isArray(replacementNode) | |||
? `[${replacementNode.map((n) => n.type)}.join(', ')]` | |||
? `[${replacementNode.map((n) => n && n.type)}.join(', ')]` |
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.
This was just for fun in case we haver have null
as a replacement node.
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.
TODO: Base class does not have a template or render, superclass does not have a template, but does have a render function. Base class will inherit superclass render
, so it should already work, but we should add a test. But what happens if the base class does render = undefined
or something dumb?
Also test: base class has template, super class has render and no template.
Also test: component has a default template ( |
program.body.unshift( | ||
bImportDeclaration({ SYMBOL__DEFAULT_TEMPLATE: '__SYMBOL__DEFAULT_TEMPLATE' }) | ||
); | ||
exposeTemplateBlock = bExposeTemplate(tmplVar, classIdentifier); |
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.
It's kind of weird to me to have tmplVar
used in two places – I think everywhere else we just clone rather than have multiple references to the same thing.
path.node.id = b.identifier('DefaultComponentName'); | ||
state.lwcClassName = 'DefaultComponentName'; | ||
} | ||
// Assume everything with a superclass is an LWC component |
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.
This is not 100% right because it should be API-versioned... but I can't think of how this could fail for older API versions right now.
Details
All of these are now supported! Matching the behavior of
engine-server
, anything with a superclass is now parsed as an LWC component.Because a component can inherit a template from its superclass, we use a new symbol to attach the template to the class static.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item