Consider using peerDependencies for LWC

[nolanlawson]

Currently @salesforce/sfdx-lwc-jest has an explicit dependency on these core LWC packages:

• @lwc/compiler
• @lwc/engine-dom
• @lwc/engine-server
• @lwc/module-resolver
• @lwc/synthetic-shadow
• @lwc/wire-service

And these lwc-test packages:

• @lwc/jest-preset
• @lwc/jest-resolver
• @lwc/jest-serializer
• @lwc/jest-transformer

I've seen cases where a repo ends up with mixed LWC dependencies, due to @salesforce/sfdx-lwc-jest bringing in its own version of LWC, in addition to 1) direct dependencies on LWC, and 2) dependencies from other meta-packages (like LWR). This can lead to version mismatch errors where a component is compiled with one version of LWC and run through another version's runtime.

It's less of an issue for the lwc-test dependencies, but it could still happen.

Arguably we could use peerDependencies for the above – at least for the core LWC packages. This would be a breaking change, since consumers would now need to install their own version of LWC on top of @salesforce/sfdx-lwc-jest.

We could ease the burden by requiring only the lwc package to be installed, but this would require changing all the imports from e.g. @lwc/engine-dom to lwc/engine-dom. The same may need to be done for @lwc/jest-*, which does use peerDependencies already.

[ekashida]

For some context, when we first created this package, we took into account the typical user who may not know how to manage dependencies. That said, we haven't really done a good job of keeping the LWC version up to date, outside of promoting prerelease to latest once per release. I haven't really seen any complaints about this though, outside of outdated stubs.

[nolanlawson]

Yeah it also calls into question whether we would keep the existing winter24/spring24/etc branch/tag system if the LWC version were decoupled.

[nolanlawson]

So in short, the customer adoption story would be:

1. Update to @salesforce/sfdx-lwc-jest v5+, which is a breaking change
2. Install the relevant LWC version corresponding to the @salesforce/sfdx-lwc-jest version (which uses the same winter24/spring24/etc tagging system)

For new customers, the flow would be:

yarn add -D \
    @salesforce/sfdx-lwc-jest@winter24 \
    lwc@winter24 # <-- this is new

(For @lwc/jest-* we can decide later what to do. It doesn't really have breaking changes related to LWC/Salesforce releases so it's probably fine to just have it as a dep of @salesforce/sfdx-lwc-jest.)

[jamessimone]

I don't think this is too big of a lift to ask for. import { createElement } from 'lwc'; will still be the correct syntax within test files with this proposed change, correct?

[nolanlawson]

@jamessimone Correct, yes. That will not change. The only change is that consumers of @salesforce/sfdx-lwc-jest will also need to explicitly yarn install/npm install the lwc package if they haven't already.

[jamessimone]

yeah, I mean, I'm always in favor of explicitly being able to control/pin dependencies. This seems like a net win!

[pozil]

@nolanlawson if your proposal went ahead, we would no longer need to install @salesforce/sfdx-lwc-jest with a Salesforce release tag like you shared in your last install example, right?
Shouldn't your example be like:

yarn add -D \
    @salesforce/[email protected] \  # <-- this version/tag is no longer tied to a Salesforce release
    lwc@winter24 # <-- this is new

[nolanlawson]

@pozil Sadly yes, you would still need the correct release tag for @salesforce/sfdx-lwc-jest, due to two things.

First, there is the expectedApiVersion:

sfdx-lwc-jest/src/config.js

Line 47 in 37d0f0b

const expectedApiVersion = '60.0';

Second, there are the lightning-stubs which do occasionally change release-to-release.

Perhaps we can find some other way to determine the expectedApiVersion, and then hope that lightning-stubs doesn't change much (it does seem infrequent).

[nolanlawson]

Alternatively, we could get rid of expectedApiVersion entirely. It looks like the whole goal is just to validate that the user has the API version we expect, which could be pointless if @salesforce/sfdx-lwc-jest is decoupled from versioning entirely:

sfdx-lwc-jest/src/utils/test-runner.js

Lines 24 to 28 in 37d0f0b

	if (apiVersion !== expectedApiVersion) {
	error(
	`Invalid sourceApiVersion found in sfdx-project.json. Expected ${expectedApiVersion}, found ${apiVersion}`,
	);
	}

[nolanlawson]

We (LWC team) talked about this, and concluded:

1. This should probably wait until we can use a single lwc package, to avoid customers having to install a half-dozen @lwc/* packages with identical versions. This would require moving this package to ESM.
2. The existing expectedApiVersion test is low-value, because 1) it assumes customers are frequently updating the @salesforce/sfdx-lwc-jest package, which is not guaranteed, and 2) all it does is prevent customers from running local Jest tests against a stale version of LWC (relative to the version used in core).

[nolanlawson]

Looking into this more deeply, we may not even need to convert to ESM or change to a single lwc package.

This project doesn't actually rely on any LWC dependencies (other than @lwc/jest-*) – you can delete all the others and the tests will still pass.

The only reliance is here:

sfdx-lwc-jest/src/resolver.js

Lines 79 to 81 in c3ace89

	if (modulePath === 'lwc') {
	return require.resolve('@lwc/engine-dom');
	}

So this basically instructs Jest that, if it sees the 'lwc' package, it should resolve it to '@lwc/engine-dom'.

So essentially, consumers of this package should just be able to install lwc and @salesforce/sfdx-lwc-jest, and everything will continue to work as before.

There are no issues with bundlers/tools that complain about importing implicit transitive dependencies (e.g. Yarn Berry), because if you were able to import @lwc/engine-dom before, it was through @salesforce/sfdx-lwc-jest (transitive dep), and now it would be through lwc (transitive dep). Nothing has changed.

The main thing we will need to do is update SF cli so that a new project is templated with both @salesforce/sfdx-lwc-jest and lwc. That file is here.

We will also need to do a major version bump and explain that you have to install lwc separately now.

We should also get rid of the expectedApiVersion check because it seems quite low-value.

[ekashida]

@nolanlawson sounds like a solid plan. I think the only remaining issue is that we would theoretically be losing the ability to poke the user into updating their version every release.

@pozil do you have more context on the purpose of the expectedApiVersion check? I vaguely remember us putting that in place as a reminder to update, but its comparing the expected api version with the api version of the project, and I'm not sure what the api version of the project is used for.

[pozil]

@ekashida I would remove this check. I agree with @nolanlawson, I never saw the value of this check.
All of our sample apps CI and local dev scripts ran with the --skipApiVersionCheck flag from day one and we never had issues with this check disabled.

[pozil]

I forgot to mention that one of the main drivers for disabling this check was the fact that we often were waiting for a new version of sfdx-lwc-jest during Salesforce pre-releases and this would crash our CI. I remember that I had ask for help on Slack a number of times as the release cadence of sfdx-lwc-jest wasn't predictable enough vs the Salesforce releases.
If the Salesforce version isn't really used in sfdx-lwc-jest, this one more reason to drop this check :)

[nolanlawson]

We can drop the check even without refactoring our LWC dependencies. Based your feedback @pozil I would accept a PR that simply did that. 🙂

[pozil]

@nolanlawson I took your offer and submitted PR #370 😉

[nolanlawson]

Now that #370 is merged, I think the next step is just to make @lwc/engine-dom a peerDependency.

[nolanlawson]

OK now that I look at this, @lwc/jest-preset has a peerDependency on @lwc/compiler, @lwc/engine-dom, @lwc/engine-server, and @lwc/synthetic-shadow.

So it's already the case that you need particular versions of those deps when using @lwc/jest-preset.