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

fix: replace barrel exports from lwc with @lwc/ssr-runtime #5034

Merged
merged 9 commits into from
Dec 17, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Barrel exporting from 'lwc' with alias should work
export * as foo from 'lwc';
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<x-cmp>
<template shadowrootmode="open">
<div>
function getComponentDef
function isComponentConstructor
function createContextProvider
function readonly
function setFeatureFlagForTest
function unwrap
function createElement
function renderComponent
</div>
</template>
</x-cmp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const tagName = 'x-cmp';
export { default } from 'x/cmp';
export * from 'x/cmp';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template>
<div lwc:inner-html={imports}></div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { foo } from '../../../barrel.js';
import { importsToString } from '../../../../shared.js';

export default class extends foo.LightningElement {
imports = importsToString([
foo.getComponentDef,
foo.isComponentConstructor,
foo.createContextProvider,
foo.readonly,
foo.setFeatureFlagForTest,
foo.unwrap,
foo.createElement,
foo.renderComponent,
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
imports = importsToString([
foo.getComponentDef,
foo.isComponentConstructor,
foo.createContextProvider,
foo.readonly,
foo.setFeatureFlagForTest,
foo.unwrap,
foo.createElement,
foo.renderComponent,
]);
imports = Object.values(foo).map(fn => fn.name);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a number of differences between v1/v2 but they don't impact the set of functions that are expected to work when imported so I didn't base it off the entire import. See below (v2 left, v1 right)

image

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Barrel exporting from 'lwc' should work
export * from 'lwc';
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<x-cmp>
<template shadowrootmode="open">
<div>
function getComponentDef
function isComponentConstructor
function createContextProvider
function readonly
function setFeatureFlagForTest
function unwrap
function createElement
function renderComponent
</div>
</template>
</x-cmp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const tagName = 'x-cmp';
export { default } from 'x/cmp';
export * from 'x/cmp';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template>
<div lwc:inner-html={imports}></div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import {
LightningElement,
getComponentDef,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
} from '../../../barrel.js';
import { importsToString } from '../../../../shared.js';

export default class extends LightningElement {
imports = importsToString([
getComponentDef,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
]);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Barrel exporting from 'lwc' should work
// These imports are the ones that are allowed by the no-disallowed-lwc-imports eslint rule
// Ref: https://github.com/salesforce/eslint-plugin-lwc/blob/34911de749e20cabbf48f5585c92a4b62d082a41/lib/rules/no-disallowed-lwc-imports.js#L11
export {
LightningElement,
getComponentDef as getComponentDefAlias,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
} from 'lwc';
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<x-cmp>
<template shadowrootmode="open">
<div>
function getComponentDef
function isComponentConstructor
function createContextProvider
function readonly
function setFeatureFlagForTest
function unwrap
function createElement
function renderComponent
</div>
</template>
</x-cmp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const tagName = 'x-cmp';
export { default } from 'x/cmp';
export * from 'x/cmp';
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<template>
<div lwc:inner-html={imports}></div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import {
LightningElement,
getComponentDefAlias,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
} from '../../../barrel.js';
import { importsToString } from '../../../../shared.js';

export default class extends LightningElement {
imports = importsToString([
getComponentDefAlias,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
]);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export function importsToString(imports) {
return imports
.map((i) =>
i
.toString()
.split('(')[0]
// renderComponent is aliased here: https://github.com/salesforce/lwc/blob/5d01843a7733a03b9ccb59a70ad64af955f15b88/packages/%40lwc/ssr-runtime/src/index.ts#L31
.replace('async function serverSideRenderComponent', 'function renderComponent')
)
.join('\n ');
}
Copy link
Collaborator

@nolanlawson nolanlawson Dec 16, 2024

Choose a reason for hiding this comment

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

I think we could make this less fragile by using a simple typeof instead of stringifying the entire function. I also tend to prefer inlining these kinds of things into fixtures instead of having shared utils (code reuse is not super important for fixture tests).

Copy link
Contributor

Choose a reason for hiding this comment

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

fn.name should also work (unless rollup mangles it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, that works thanks

8 changes: 7 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-js/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { traverse, builders as b, is } from 'estree-toolkit';
import { parseModule } from 'meriyah';

import { transmogrify } from '../transmogrify';
import { replaceLwcImport } from './lwc-import';
import { replaceLwcImport, replaceNamedLwcExport, replaceAllLwcExport } from './lwc-import';
import { catalogTmplImport } from './catalog-tmpls';
import { catalogStaticStylesheets, catalogAndReplaceStyleImports } from './stylesheets';
import { addGenerateMarkupFunction } from './generate-markup';
Expand All @@ -23,6 +23,12 @@ import type { CompilationMode } from '@lwc/shared';

const visitors: Visitors = {
$: { scope: true },
ExportNamedDeclaration(path) {
replaceNamedLwcExport(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is replaceNamedLwcExport used anywhere else? Can we just inline it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not, but same goes for replaceLwcImport so I was just keeping it consistent. Also feel like this file could get a little busy if we added everything inline?

},
ExportAllDeclaration(path) {
replaceAllLwcExport(path);
},
ImportDeclaration(path, state) {
if (!path.node || !path.node.source.value || typeof path.node.source.value !== 'string') {
return;
Expand Down
41 changes: 39 additions & 2 deletions packages/@lwc/ssr-compiler/src/compile-js/lwc-import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { builders as b } from 'estree-toolkit';

import type { ImportDeclaration } from 'estree';
import type { ImportDeclaration, ExportNamedDeclaration, ExportAllDeclaration } from 'estree';
import type { NodePath } from 'estree-toolkit';
import type { ComponentMetaState } from './types';

Expand All @@ -18,7 +18,7 @@ import type { ComponentMetaState } from './types';
* 2. it makes note of the local var name associated with the `LightningElement` import
*/
export function replaceLwcImport(path: NodePath<ImportDeclaration>, state: ComponentMetaState) {
if (!path.node || path.node.source.value !== 'lwc') {
if (!path.node || !isLwcSource(path)) {
return;
}

Expand All @@ -35,3 +35,40 @@ export function replaceLwcImport(path: NodePath<ImportDeclaration>, state: Compo

path.replaceWith(b.importDeclaration(path.node.specifiers, b.literal('@lwc/ssr-runtime')));
}

/**
* This handles lwc barrel exports by replacing "lwc" with "@lwc/ssr-runtime"
*/
export function replaceNamedLwcExport(path: NodePath<ExportNamedDeclaration>) {
if (!path.node || !isLwcSource(path)) {
return;
}

path.replaceWith(
b.exportNamedDeclaration(
path.node.declaration,
path.node.specifiers,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's structuredClone these for safety. Ditto below for path.node.exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thank you, I updated Dales replaceLwcImport impl above to keep it consistent.

b.literal('@lwc/ssr-runtime')
)
);
}

/**
* This handles all lwc barrel exports by replacing "lwc" with "@lwc/ssr-runtime"
*/
export function replaceAllLwcExport(path: NodePath<ExportAllDeclaration>) {
if (!path.node || !isLwcSource(path)) {
return;
}

path.replaceWith(b.exportAllDeclaration(b.literal('@lwc/ssr-runtime'), path.node.exported));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't correctly handle the case of

export * as foo from 'lwc'

which is not the same as

export * from 'lwc'

In this case, we would need to maintain the foo identifier as well (try this example on https://astexplorer.net/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, sorry I missed this


/**
* Utility to determine if a node source is 'lwc'
*/
function isLwcSource(
path: NodePath<ExportAllDeclaration | ExportNamedDeclaration | ImportDeclaration>
): boolean {
return path.node?.source?.value === 'lwc';
}