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

[jhefferman-sfdc]

Details

Does this pull request introduce a breaking change?

• 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• 🤞 No, it does not introduce an observable change.

GUS work item

W-16984771

[nolanlawson]

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>

[jhefferman-sfdc]

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)

[nolanlawson]

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.

[jhefferman-sfdc]

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)?

[nolanlawson]

Yep looks like it.

[nolanlawson]

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/).

[jhefferman-sfdc]

thank you, sorry I missed this

[nolanlawson]

I'd still love for us to do something with these rather than just logging them. E.g. exposing it on the component so that the template can prove that it rendered something, and that the exports we got from here are the same ones exported from @lwc/ssr-runtime (i.e. no duplicate modules).

Also, logging here will make our tests more noisy. We already have noisy logs, but this just adds to the problem. We could do something like Promise.resolve(...) to avoid the compiler removing it.

[jhefferman-sfdc]

I'd still love for us to do something with these rather than just logging them. E.g. exposing it on the component so that the template can prove that it rendered something

It's a little crude, but what do you think about exposing them in the template via inner-html? Example here. The function implementations would differ between v1 and v2 ofc so I trimmed them... E.g. getComponentDef in V1 vs V2 (stub)

Also, logging here will make our tests more noisy. We already have noisy logs, but this just adds to the problem.

Ok makes sense. See above.

[nolanlawson]

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).

[wjhsf]

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

[jhefferman-sfdc]

Nice, that works thanks

[nolanlawson]

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

[jhefferman-sfdc]

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

[nolanlawson]

Small nits, LGTM

[wjhsf]

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

[wjhsf]

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);

[jhefferman-sfdc]

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)

[wjhsf]

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

[jhefferman-sfdc]

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?