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

refactor(template-compiler): remove apis template fn argument #2758

Closed
wants to merge 3 commits into from

Conversation

jodarove
Copy link
Contributor

@jodarove jodarove commented Mar 23, 2022

Details

This PR exposes the renderApi into the lwc module, so we can remove the $api argument in the template function of the module in favor of hoisting the used apis outside the template function.

The changes on this PR are required for #2688.

Commits:

  • 0f98091 contains the functional changes (desired)
  • 4bff6d1 updates the snapshots.
  • 25f64ec adds support for the legacy API (act compiler).

Note: As part of this PR, I noticed two things:

  1. In our codebase, compileToFunction is only used for test, and only 4 tests, opened refactor(template-compiler): remove compileToFunction #2757 to remove it.
  2. We need to support the legacy tpl format too because of the ACT compiler or (my preference) we could modify the ACT compiler so it matches the new compilation output.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce an observable change. (as it is)
  • 🚨 Yes, it does introduce a breaking change. (desired, which needs act compiler modifications)

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change. (as it is)
  • ⚠️ Yes, it does include an observable change. (if we decide to update the ACT compiler)

GUS work item

Comment on lines +177 to +188
return [
t.variableDeclaration('const', [
t.variableDeclarator(
t.objectPattern(
Object.keys(codeGen.usedApis).map((name) =>
t.assignmentProperty(t.identifier(name), codeGen.usedApis[name])
)
),
t.identifier(RENDER_API)
),
]),
];
Copy link
Member

Choose a reason for hiding this comment

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

From a tree-shaking perspective, it is probably better to export each render API independently. This would also remove the need to use an object spread at the top of each generated template. That said I am wondering if the tree-shaking benefits are outweighed by the increased output size for AMD.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also adds some unnecessary boilerplate in compat mode:

lwc.renderApi._ES5ProxyType ? lwc.renderApi.get("d") : lwc.renderApi.d

That said I am wondering if the tree-shaking benefits are outweighed by the increased output size for AMD.

I don't understand this. Could you explain?

Copy link
Member

Choose a reason for hiding this comment

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

That said I am wondering if the tree-shaking benefits are outweighed by the increased output size for AMD.

I don't understand this. Could you explain?

Good catch @nolanlawson. I was mistaken, there is no increase in AMD module size. I was under the impression that using named imports would negatively impact the AMD module size by adding new entries in the dependency list (the first argument of the define function). But it's wrong, the list contains the dependency module names and not the named imports for each dependency.

import { createVElement, createVText, createVComponent } from 'lwc';

createVElement();
createVText();
createVComponent();
define(['lwc'], (function (lwc) { 'use strict';
	lwc.createVElement();
	lwc.createVText();
	lwc.createVComponent();
}));

@nolanlawson
Copy link
Collaborator

I second PM's comment about tree-shaking. That would be especially important for the off-core (e.g. B2C) scenario, where a page might have a handful of LWC components that only use a few functions from the renderApi. It would be great if we could tree-shake out all the rest.

I do wonder, though, if we are overloading the lwc package by adding all the render APIs to it. Maybe something like this would be cleaner:

import { text, html } from `@lwc/renderer`

The tricky part is that the @lwc/renderer would have to be replaced at compile time. To be fair, though, we are already doing this with the lwc package (which could mean either @lwc/engine-dom or @lwc/engine-server).

@pmdartus
Copy link
Member

I do wonder, though, if we are overloading the lwc package by adding all the render APIs to it.

I used to share the same feeling, but at the end of the day, those named exports are injected at compile time. Adding an indirection like @lwc/renderer would certainly produce cleaner code, but outside this, it is not obvious to me the other added benefits of this approach.

@jodarove jodarove force-pushed the jodarove/remove-apis-from-tpl branch from 25f64ec to 798a40b Compare March 25, 2022 16:37
@jodarove
Copy link
Contributor Author

closed in favor of #2781

@jodarove jodarove closed this Apr 11, 2022
@jodarove jodarove deleted the jodarove/remove-apis-from-tpl branch September 16, 2022 20:00
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.

3 participants