CLI: refactor to simplify works with multiple package managers #11074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaetanmaisse
force-pushed
the
refactor-cli
branch
2 times, most recently
from
fbaa504
to
2c153cf
Compare
… subclasses for NPM, Yarn and Yarn 2. The goal of this will be to use the Factory Pattern to encapsulate everything specific to a package manager in a single class. Then a factory will return an instance of NPM, Yarn or Yarn 2 proxy class according to the package manager used/prefered in end project. All CLI code will be package manager agnostic and will just call functions of JsPackageManager interface.
It returns an NPMProxy, YarnProxy or Yarn2Proxy according to user project setup: Are `yarn` or `npm` command available? Is there any `yarn.lock` file?
It avoids to use a ternary expression to define the displayed string and encapsulate specific package managers behaviors in their own classes.
As `hasYarn` function is called when `npm_init.ts` is parsed and loaded there are some issue with mock in unit tests.
…ger class and subclasses
Also rename it to `addDependencies` (a.k.a. `yarn add`) to avoid confusion with `installDepsFromPackageJson` (a.k.a. `yarn install`).
gaetanmaisse
force-pushed
the
refactor-cli
branch
from
2c153cf
to
2ec49eb
Compare
… behavior in NPM, Yarn 1 and Yarn 2 classes.
All this variables were used to check if user was running Yarn and do this accordingly. Now we use the factory to get the "right" version of JsPackageManager instance so there is no need to worry anywhere about that.
gaetanmaisse
force-pushed
the
refactor-cli
branch
2 times, most recently
from
2195244
to
f0db5dc
Compare
gaetanmaisse
force-pushed
the
refactor-cli
branch
from
f0db5dc
to
1504dd5
Compare
…rn` or `npm` calls It move the `spawnSync` usage to only a single place which is simpler to maintain. It also simplify testing as there is no more need to mock an external dep
…ng `package.json` Doing this just after having read the file enable us to remove duplication and unneeded calls to `writePackageJson`.
gaetanmaisse
force-pushed
the
refactor-cli
branch
from
1504dd5
to
9dc7c55
Compare
gaetanmaisse
requested review from
ndelangen,
shilman and
stijnkoopal
as code owners
|
|
PiDelport
reviewed
Jul 1, 2021
| return parsedOutput; | ||
| } | ||
| } catch (e) { | ||
| throw new Error(`Unable to find versions of ${packageName} using yarn`); |
There was a problem hiding this comment.
This refers to yarn instead of npm.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I did
I introduced a
JsPackageManagerclass and specialized subclasses for NPM, Yarn 1, and Yarn 2. The goal of this is to use the Factory Pattern to encapsulate everything specific to a package manager in a single class.The factory returns an instance of NPM, Yarn 1, or Yarn 2 proxy class according to the package manager used/preferred in the end project.
All CLI's code will be package manager agnostic and will just call functions of
JsPackageManagerinterface.The PR is quite big but reading commit by commit should be ok as it really follows how I worked on this.
There is still to do to centralize things but I prefer to open a working PR now and continue later than working on a branch for weeks and then have a 895612456756 conflicts to solve. 🙃
How to test