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

Consider using peerDependencies for LWC #349

Open
nolanlawson opened this issue Feb 14, 2024 · 18 comments
Open

Consider using peerDependencies for LWC #349

nolanlawson opened this issue Feb 14, 2024 · 18 comments
Labels
enhancement New feature or request

Comments

@nolanlawson
Copy link
Contributor

nolanlawson commented Feb 14, 2024

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.

@nolanlawson nolanlawson added the enhancement New feature or request label Feb 14, 2024
@ekashida
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

@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
Copy link

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

@pozil
Copy link
Contributor

pozil commented Feb 16, 2024

@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/sfdx-lwc-jest@5.0.0 \  # <-- this version/tag is no longer tied to a Salesforce release
    lwc@winter24 # <-- this is new

@nolanlawson
Copy link
Contributor Author

@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:

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
Copy link
Contributor Author

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:

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

@nolanlawson
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

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
Copy link
Member

@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
Copy link
Contributor

pozil commented Apr 26, 2024

@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
Copy link
Contributor

pozil commented Apr 27, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

pozil commented May 23, 2024

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

@nolanlawson
Copy link
Contributor Author

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

@nolanlawson
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants