-
Notifications
You must be signed in to change notification settings - Fork 395
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: new compiler targeting SSR #3880
Conversation
Here's a comparison of the two compilation outputs of a simple component: Today's compiler output: 'use strict';
Object.defineProperty(exports, '__esModule', { value: true });
var lwc = require('lwc');
const $fragment1 = lwc.parseFragment`<div class="foo bar foo-bar${0}" data-foo="foo" style="color: red; background-color: blue;"${2}></div>`;
function tmpl($api, $cmp, $slotset, $ctx) {
const {st: api_static_fragment} = $api;
return [api_static_fragment($fragment1(), 1)];
/*LWC compiler v4.0.1*/
}
var _tmpl = lwc.registerTemplate(tmpl);
tmpl.stylesheets = [];
tmpl.stylesheetToken = "lwc-4ol5nhdt6n2";
tmpl.legacyStylesheetToken = "x-attribute-static_attribute-static";
lwc.freezeTemplate(tmpl);
class AttributeStatic extends lwc.LightningElement {
/*LWC compiler v4.0.1*/
}
var attributeStatic = lwc.registerComponent(AttributeStatic, {
tmpl: _tmpl,
sel: "x-attribute-static",
apiVersion: 60
});
const tagName = 'x-attribute-static';
exports.default = attributeStatic;
exports.tagName = tagName; SSR compiler output: 'use strict';
var runtime = require('@lwc/ssr-compiler/runtime');
require('@lwc/shared');
async function* tmpl(props, attrs, slotted, Cmp, instance) {
if (Cmp.renderMode !== 'light') {
yield '<template shadowroot="open">';
}
yield "<div class=\"foo bar foo-bar\" data-foo=\"foo\" style=\"color: red; background-color: blue;\"></div>";
if (Cmp.renderMode !== 'light') {
yield '</template>';
}
}
class AttributeStatic extends runtime.LightningElement {}
async function* generateMarkup(tagName, props, attrs, slotted) {
const instance = new AttributeStatic({
tagName: tagName.toUpperCase()
});
Object.assign(instance, props);
yield `<${tagName}`;
yield* runtime.renderAttrs(attrs);
yield '>';
const tmplFn = tmpl ?? runtime.fallbackTmpl;
yield* tmplFn(props, attrs, slotted, AttributeStatic, instance);
yield `</${tagName}>`;
}
const tagName = 'x-attribute-static';
exports.generateMarkup = generateMarkup;
exports.tagName = tagName; |
d2a051e
to
b1b5222
Compare
798b19d
to
e3de526
Compare
packages/@lwc/engine-server/src/__tests__/fixtures/attribute-aria/index.js
Show resolved
Hide resolved
cb499b4
to
cb56d14
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.
mostly small things, because evaluating the big picture is hard when you make the picture too big to fit on the screen without an hour of scrolling. 😜
(part 1)
const ast = parseModule(src, { | ||
module: true, | ||
next: true, | ||
}) as EsProgram; |
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 a shame meriyah doesn't use @types/estree
but exposes it's own types which are slightly different. These subtle differences could end up causing bigger issues.
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.
Based on this issue it sounds like there's openness to refactoring on top of estree types; it's just a lot of work. The thing is, acorn also doesn't use estree types and instead reimplements them. So it seems like this is just a thing with JS parsers.
I may take a peak at the meriyah source to see how hard it'd be to contribute this change. And long term, despite the perf wins with meriyah, we can always use acorn like we're doing elsewhere.
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'm also a bit concerned about having so many JS parsers in our packages in general. We already have Babel and Acorn and estree-walker, now we would be adding meriyah. Just an opportunity for any one of those things to throw breaking changes at us over time.
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 remember why I brought in meriyah - acorn doesn't support Js features until they are stage 4. This includes decorators. So it was a choice between babel+acorn or meriyah. I hate the double-transform, especially when compilation might be in the hot path, which is why I went with meriyah. I'm open to alternatives.
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.
Minor nits. LGTM, anything else can be addressed in later PRs.
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 need a lot more time to grok this PR (this is one of the more theory-dense PRs we've had in a while!) but this is my initial feedback.
const ast = parseModule(src, { | ||
module: true, | ||
next: true, | ||
}) as EsProgram; |
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'm also a bit concerned about having so many JS parsers in our packages in general. We already have Babel and Acorn and estree-walker, now we would be adding meriyah. Just an opportunity for any one of those things to throw breaking changes at us over time.
63f15e3
to
3f1677a
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.
If we're going to iterate on this a lot before we release it, should we have it live on a feature branch instead of master?
} else if (typeof value === 'boolean') { | ||
return [bYield(b.literal(` ${name}`))]; | ||
} | ||
throw new Error(`Unknown attr/prop literal: ${type}`); |
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.
Will never reach this. value
is of type stirng | boolean
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.
True. I think I like having the guard for cases when we're getting lazy about typing and we're casting or using any
somewhere.
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 fun type guard hack you can do here is
const _valueTypeCheck: never = value
You can only assign never
to never
, so if the type of value
or the preceding conditionals ever change, then this becomes a type error, so you can catch it sooner!
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.
Too late now, but it's a bit weird that the error is an unknown value
, but the message is refers to type
. type
is always "Literal"
, so the error message wouldn't be super helpful here if we accidentally pass, for example, a numeric literal or something.
49a3436
to
5b2f725
Compare
5b2f725
to
3377347
Compare
This introduces a new compiler to the
@lwc
namespace. Today, there is a single compiler that generates per-component artifacts; these artifacts are invoked whether you're running against@lwc/engine-dom
or@lwc/engine-server
. This fact brings with it many constraints, two of which are particularly noteworthy when considering SSR use cases:The introduction of this new compiler would mean that LWCs are compiled twice:
@lwc/engine-dom
, utilizes VDOM for diffing and rendering, and interacts with browser elements & other browser constructs.() => AsyncGenerator<string, never, void>
.Currently, the new compiler is wired up to the snapshot tests of
@lwc/engine-server
. It is only passing roughly 40% of the tests at present, and notably does not yet support slotted content, context, and various sundry. However, the code has taken shape over the course of the spike, such that it is becoming increasingly evident how those gaps may be closed.An initial perf benchmark can be found here
I'm aiming to merge this PR such that we can iterate towards 100% compliance with the
@lwc/engine-server
test suite in smaller, incremental PRs. While it is possible to run the tests locally, the@lwc/ssr-compiler
tests do not run duringyarn run test
and do not cause CI to fail. Once we're at 100%, we can turn on coverage officially.