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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40f58f0
fix: replace barrel from `lwc` package with '@lwc/ssr-runtime'
jhefferman-sfdc Dec 10, 2024
8777a1d
fix: handle * barrel case and corresponding tests
jhefferman-sfdc Dec 11, 2024
be6a08b
fix: function naming
jhefferman-sfdc Dec 11, 2024
87eb3c5
fix: barrel import test parity
jhefferman-sfdc Dec 11, 2024
ed28cf1
fix: include optional exported alias for export all declaration repla…
jhefferman-sfdc Dec 11, 2024
3794535
chore: explain function name massaging in test
jhefferman-sfdc Dec 12, 2024
4b733da
fix: deep clone objects and optimize tests
jhefferman-sfdc Dec 16, 2024
7478dcc
fix: remove unused shared file
jhefferman-sfdc Dec 17, 2024
6a486f4
fix: merged master
jhefferman-sfdc Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/error.txt
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/expected.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<x-cmp>
<template shadowrootmode="open">
</template>
</x-cmp>
14 changes: 14 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/imports.js
View file
Open in desktop
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,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
} from 'lwc';
Copy link
Collaborator

@nolanlawson nolanlawson Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Some more tests to add:

  • export * as lwc from 'lwc'
  • export { doesNotExist } from 'lwc'
  • export { registerComponent } from 'lwc' // should be disallowed

We don't have to be perfect about this because there is already ESLint validation on LEX, but we should at least make sure that these cases work correctly.

Also a good way to confirm that the import is actually working is something like

import { LightningElement } from './barrel.js'
// ...
constructor() {
  super()
  this.isInstanceOfLightningElement = this instanceof LightningElement
}
<template>
    {isInstanceOfLightningElement}
</template>

Copy link
Contributor Author

@jhefferman-sfdc jhefferman-sfdc Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

man the reviews on this team are so fast :D thank you!

export * as lwc from 'lwc'

Thanks, realized exportAllDeclaration is missing and valid, working on it now.

export { doesNotExist } from 'lwc'

Ok got you. I didn't see any coverage like that for imports, I will add some.

export { registerComponent } from 'lwc' // should be disallowed

Is this validation handled in the compiler pass or at runtime? I was confused there perhaps. 4674)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I got mixed up: we don't do any validation of lwc imports/exports in this repo. It's in eslint-plugin-lwc which runs on-platform already.

I thought we did some validation here, but just checked and couldn't find any.

So I think we only need to test export * as lwc then. import { doesNotExist } will likely throw an error at the Rollup level, so not worth testing.

Copy link
Contributor Author

@jhefferman-sfdc jhefferman-sfdc Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had a similar confusion - Not sure if this is what you were thinking of but there was some sort of validation in the compiler pass and this WI dated from then (4617). But later it was moved to runtime only (4674)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep looks like it.

3 changes: 3 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/index.js
View file
Open in desktop
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';
2 changes: 2 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/modules/x/cmp/cmp.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<template>
</template>
31 changes: 31 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/barrel-export-lwc/modules/x/cmp/cmp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Everything else can be imported in a helper, so we must check in the helper
import {
LightningElement,
getComponentDef,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent,
} from '../../../imports.js';

// "Using" the imports so they don't get removed by the compiler
console.log(
LightningElement,
// The LWC compiler doesn't let us use decorators like this, so we don't need to check them here
// api,
// track,
// wire,
getComponentDef,
isComponentConstructor,
createContextProvider,
readonly,
setFeatureFlagForTest,
unwrap,
createElement,
renderComponent
);

export default class extends LightningElement {}
5 changes: 4 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-js/index.ts
View file
Open in desktop
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, replaceNamedLwcImport } from './lwc-import';
import { catalogTmplImport } from './catalog-tmpls';
import { catalogStaticStylesheets, catalogAndReplaceStyleImports } from './stylesheets';
import { addGenerateMarkupFunction } from './generate-markup';
Expand All @@ -23,6 +23,9 @@ import type { CompilationMode } from '@lwc/shared';

const visitors: Visitors = {
$: { scope: true },
ExportNamedDeclaration(path) {
replaceNamedLwcImport(path);
},
ImportDeclaration(path, state) {
if (!path.node || !path.node.source.value || typeof path.node.source.value !== 'string') {
return;
Expand Down
19 changes: 18 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-js/lwc-import.ts
View file
Open in desktop
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 } from 'estree';
import type { NodePath } from 'estree-toolkit';
import type { ComponentMetaState } from './types';

Expand Down Expand Up @@ -35,3 +35,20 @@ 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 replaceNamedLwcImport(path: NodePath<ExportNamedDeclaration>) {
if (!path.node || path.node.source?.value !== 'lwc') {
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')
)
);
}