[rush] Copy resolutions section from source package.json for yarn #1360

MasterLambaster · 2019-07-06T07:38:29Z

Copy resolutions to the base package.json and to the project specific auto generated files.
Fixes #831.

msftclas · 2019-07-06T07:38:41Z

All CLA requirements met.

apps/rush-lib/src/api/PackageJsonEditor.ts

apps/rush-lib/src/logic/InstallManager.ts

libraries/node-core-library/src/IPackageJson.ts

common/changes/@microsoft/rush/yarn-resolutions_2019-07-06-07-35.json

common/changes/@microsoft/node-core-library/yarn-resolutions_2019-07-06-07-35.json

apps/rush-lib/src/logic/InstallManager.ts

MasterLambaster · 2019-07-18T09:08:33Z

@iclanton The latest change to use index signature is causing issues on CI but not locally. Local run on node 8.16.0 passes successfully.

[12:06:45] Project @microsoft/rush-lib version:5.10.1
[12:06:45] Build tools version:3.9.26
[12:06:45] Node version:v8.16.0
[12:06:45] Total duration:30 s

With tslint version 5.12.1 and typescript version3.2.4.
Is there easy way to get typescript version on CI?
Or should I rollback those changes?

iclanton · 2019-07-25T06:36:47Z

Sorry I didn't see this when you updated it. I'll take a look tomorrow when I'm at the office.

iclanton · 2019-07-26T00:56:27Z

The typescript version is 3.4.

I fixed the issue you were seeing.

rakeshpatnaik · 2019-09-18T17:38:11Z

@MasterLambaster - Can you take a look and respond to the open comments?

chriscasola · 2019-10-17T17:57:35Z

@MasterLambaster @iclanton is there more work left here? I'd be happy to contribute, this change would be extremely useful to us.

MasterLambaster · 2019-10-21T17:07:17Z

For whatever reasons there was one unresolved but yet redundant request open. It should be all good now.

chriscasola · 2019-11-05T21:16:14Z

@iclanton can this be reviewed again?

chriscasola · 2019-12-03T21:25:26Z

@iclanton any update?

chriscasola · 2020-01-02T20:33:39Z

@iclanton @octogonz Can I do anything to help get this merged?

chriscasola · 2020-01-14T20:38:06Z

@iclanton @octogonz any update? I can rebase this and open a new PR if it will be looked at.

octogonz · 2020-01-24T22:37:47Z

@apostolisms Could we get someone to review this? Overall the basic idea behind this PR seems sound to me. (Copy the resolutions from the project's package.json into the temp-project's package.json.)

If I remember right, this is the only way for Yarn to work around the @types versioning issue (yarnpkg/yarn#6695). So it's an important fix.

Co-Authored-By: Ian Clanton-Thuon <[email protected]>

iclanton · 2020-01-29T08:01:43Z

I just put together a repo to test this change and it doesn't seem to be working for me.

Here's my test repo:

This project has a "resolutions" field. and I checked in the yarn.lock file from a normal yarn install in that project's folder. Notice that z-schema is resolved to 5.18.3, when it would normally be 5.18.4.

However, no analogous resolution shows up in the common/config/rush/yarn.link file: z-schema is resolved to 5.18.4.

Does someone have a repo where this is working?

MasterLambaster · 2020-01-29T09:16:50Z

That's correct behavior.
Resolutions are applied only on the project level. I.e. that'll mandate version only within project1.
We've discussed behavior you're referencing to here( For whatever reasons conversation link does not work)
In order to support global resolution we need either to combine all resolutions from all packages and deal with conflicts, either do it some other way.

iclanton · 2020-01-29T18:41:17Z

@MasterLambaster - but the resolution does seem to even be applying on the project level. I've specified a resolution in proj1, but it's still getting [email protected], not 3.18.3

MasterLambaster · 2020-01-29T19:21:41Z

@iclanton could you please describe how do you verify it?
yarn.lock file resolves to 3.18.3
common/temp/proj1/package.json contains

 "resolutions": {
    "**/z-schema": "3.18.3"
  }

That resolutions are never making into common/temp/package.json because of the reasons I've described earlier.

It's been quite a while since initial PR and I'm struggling to recall why even having that within package were making sense 😂

iclanton · 2020-01-29T23:35:40Z

The yarn.lock file you linked to is the one I generated by just running yarn install in /proj1. The yarn.lock file produced by this PR is here. Notice that z-schema is resolved to version 3.18.4 in that yarn.lock file.

MasterLambaster · 2020-01-30T06:12:10Z

Notice that z-schema is resolved to version 3.18.4 in that yarn.lock file.

That's absolutely correct. In order to support that I need collect all resolutions from project package.json files and put into common/temp/package.json file. That imply some conflict resolution or at least warning system for that purposes.

Do you want me to restore this functionality?
Is it enough to start with the warnings if there are conflicts with different resolution instructions or just do general case to begin with?

iclanton · 2020-01-30T09:50:44Z

Do you want me to restore this functionality?

That behavior will probably produce undesired results.

Perhaps I'm confused here. Can you provide a Rush repo where the a project's "resolutions" field changes something when running rush update with the version of Rush in this PR?

MasterLambaster · 2020-01-30T20:41:26Z

So, I've took a deeper look and tired to recall all circumstances.
The current PR have just some if none utility.
This code change only copying resolutions into projects package.json files that are placed into the project tarball. The aggregated package.json(common/temp/package.json) does not use any of those files and is generated by combining all dependencies from all packages.
I.e this PR has no effect on common/config/rush/yarn.lock which is desired outcome.

Having that said I can think of following possible solutions:

Add new rush/yarn specific field that'll control global rush resolutions
CONS: Specific field that might not meet package.json schema. Does not save from conflicts. Details are hidden within package and might be hard to find
Automatically combine all resolutions and put those into global package.json
CONS: Quite complex conflict resolution system if any, otherwise can lead to unexpected errors
Add resolutions section to yarnOptions section in rush.json
CONS: Can duplicate and force-overwrite resolutions inside packages that can have some minor side effects as well.

@iclanton do you see any other places that we can use to configure resolutions explicitly?

liyikun · 2021-05-11T00:55:08Z

When will it be merged? @octogonz

octogonz · 2021-05-13T21:45:13Z

@liyikun Lemme figure out what's up. For future reference, if a PR is getting overlooked, please let us know in the #contributor-helpline chat room.

D4N14L · 2021-05-13T22:05:12Z

apps/rush-lib/src/api/PackageJsonEditor.ts

+
+  // NOTE: The "resolutions" field is a yarn specific feature that controls package
+  // resolution override within yarn.
+  private readonly _resolutions: { [name: string]: string };


Should be a Map<string, string> to match what others are doing

Or I guess, Map<string, PackageJsonDependency> if this matches a version specifier format?

@D4N14L I agree that we should make this a complete PackageJsonEditor API rather than just a way to pass the options along. A config-file-with-data is generally preferable to a script-with-code, so for everyday problems, resolutions is actually a better design than pnpmfile.js. (Although pnpmfile.js is still a good feature for advanced problems.)

I thought about adapting the PNPM shim to apply resolutions for PNPM. But it turns out that PNPM later implemented this same feature, but they call it overrides. (pnpm/pnpm#2946) Docs are here. In the issue pnpm/pnpm#1221 they point out that NPM reinvented the same feature in their "overrides" RFC but with some differences in its design. So all 3 package managers implement this feature in slightly different ways, all using different package.json fields.

We should support PNPM overrides and recommend it over pnpmfile.js. But that's not part of this PR. (I opened #2698 to track it)

D4N14L · 2021-05-13T22:08:17Z

apps/rush-lib/src/api/PackageJsonEditor.ts

+  /**
+   * This field is a Yarn-specific feature that allows overriding of package resolution.
+   */
+  public get resolutions(): { [name: string]: string } {


ReadonlyArray<PackageJsonDependency> if that type can be supported, otherwise return the a readonly object

The structure is Record<string, string> basically the same as "dependencies" and "devDependenices", with the only difference being that the name can include glob-like patterns. I think we can model it the same way.

apps/rush-lib/src/logic/InstallManager.ts

…tions # Conflicts: # apps/rush-lib/src/api/PackageJsonEditor.ts # apps/rush-lib/src/logic/InstallManager.ts # common/reviews/api/rush-lib.api.md

…geJsonDependency tables

octogonz · 2021-05-14T05:11:40Z

@D4N14L I have addressed your feedback

apps/rush-lib/src/logic/installManager/RushInstallManager.ts

apps/rush-lib/src/api/PackageJsonEditor.ts

D4N14L

Approving to avoid blocking but comment needs to be addressed before merge

octogonz · 2021-05-14T23:13:06Z

I tested this in a Rush repo with Yarn and confirmed that "resolutions" is copied to the common/temp/projects/*/package.json files.

I also tested the error message for PNPM repos, but found that it is only printed with useWorkspaces=fase. Lemme see if we can fix that.

…son field is used without Yarn. This check only worked with useWorkspaces=false, and there may be valid use cases for packages that target multiple package managers

addressed

octogonz · 2021-05-15T01:54:53Z

Released with Rush 5.47.0

MasterLambaster requested review from apostolisms, halfnibble, iclanton, Lincoded, octogonz and sachinjoseph as code owners July 6, 2019 07:38

iclanton changed the title ~~Copy resolutions section from source package.json for yarn~~ [rush] Copy resolutions section from source package.json for yarn Jul 12, 2019

iclanton previously requested changes Jul 13, 2019

View reviewed changes

iclanton added the PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. label Jul 16, 2019

buffcode mentioned this pull request Sep 16, 2019

[rush] Add support for Yarn resolutions when using Yarn #831

Closed

apostolisms approved these changes Jan 24, 2020

View reviewed changes

MrExample and others added 5 commits January 28, 2020 23:28

Copy resolutions section from source package.json for yarn

61c1b93

Apply copy suggestions from code review

7b3684c

Co-Authored-By: Ian Clanton-Thuon <[email protected]>

Use resolutions only for yarn package manager

213ce52

Use more specific type based on code review suggestion

b1579fb

Co-Authored-By: Ian Clanton-Thuon <[email protected]>

Fix a typing.

87329b8

iclanton force-pushed the yarn-resolutions branch from 478285e to 87329b8 Compare January 29, 2020 07:30

MasterLambaster requested a review from D4N14L as a code owner January 29, 2020 07:30

D4N14L requested changes May 13, 2021

View reviewed changes

octogonz added 2 commits May 13, 2021 18:54

Merge remote-tracking branch 'remotes/origin/master' into yarn-resolu…

f1d0f7d

…tions # Conflicts: # apps/rush-lib/src/api/PackageJsonEditor.ts # apps/rush-lib/src/logic/InstallManager.ts # common/reviews/api/rush-lib.api.md

rush build

7e4347a

octogonz mentioned this pull request May 14, 2021

[rush] Support PNPM "overrides" field in package.json #2698

Closed

octogonz added 3 commits May 13, 2021 22:05

PR feedback: model PackageJsonEditor.resolutions like the other Packa…

25ecbce

…geJsonDependency tables

rush change

f3f32d9

Improve type for IPackageJson.resolutions

1483b3d

D4N14L requested changes May 14, 2021

View reviewed changes

apps/rush-lib/src/logic/installManager/RushInstallManager.ts Outdated Show resolved Hide resolved

apps/rush-lib/src/api/PackageJsonEditor.ts Outdated Show resolved Hide resolved

D4N14L approved these changes May 14, 2021

View reviewed changes

PR feedback

5b4176a

octogonz removed the PR: Waiting for author We reviewed your PR but had questions/suggestions. Let us know when you're ready for another look. label May 14, 2021

Do not report an error when the Yarn-specific "resolutions" package.j…

2db822e

…son field is used without Yarn. This check only worked with useWorkspaces=false, and there may be valid use cases for packages that target multiple package managers

octogonz approved these changes May 14, 2021

View reviewed changes

octogonz merged commit b0b9804 into microsoft:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rush] Copy resolutions section from source package.json for yarn #1360

[rush] Copy resolutions section from source package.json for yarn #1360

MasterLambaster commented Jul 6, 2019

msftclas commented Jul 6, 2019 •

edited

Loading

MasterLambaster commented Jul 18, 2019 •

edited

Loading

iclanton commented Jul 25, 2019

iclanton commented Jul 26, 2019

rakeshpatnaik commented Sep 18, 2019

chriscasola commented Oct 17, 2019

MasterLambaster commented Oct 21, 2019

chriscasola commented Nov 5, 2019

chriscasola commented Dec 3, 2019

chriscasola commented Jan 2, 2020

chriscasola commented Jan 14, 2020

octogonz commented Jan 24, 2020

iclanton commented Jan 29, 2020 •

edited

Loading

MasterLambaster commented Jan 29, 2020 •

edited

Loading

iclanton commented Jan 29, 2020

MasterLambaster commented Jan 29, 2020 •

edited

Loading

iclanton commented Jan 29, 2020

MasterLambaster commented Jan 30, 2020 •

edited

Loading

iclanton commented Jan 30, 2020

MasterLambaster commented Jan 30, 2020

liyikun commented May 11, 2021

octogonz commented May 13, 2021

D4N14L May 13, 2021

D4N14L May 13, 2021

octogonz May 14, 2021 •

edited

Loading

octogonz May 14, 2021

D4N14L May 13, 2021

octogonz May 14, 2021

octogonz May 14, 2021

octogonz commented May 14, 2021

D4N14L left a comment

octogonz commented May 14, 2021

octogonz commented May 15, 2021

[rush] Copy resolutions section from source package.json for yarn #1360

[rush] Copy resolutions section from source package.json for yarn #1360

Conversation

MasterLambaster commented Jul 6, 2019

msftclas commented Jul 6, 2019 • edited Loading

MasterLambaster commented Jul 18, 2019 • edited Loading

iclanton commented Jul 25, 2019

iclanton commented Jul 26, 2019

rakeshpatnaik commented Sep 18, 2019

chriscasola commented Oct 17, 2019

MasterLambaster commented Oct 21, 2019

chriscasola commented Nov 5, 2019

chriscasola commented Dec 3, 2019

chriscasola commented Jan 2, 2020

chriscasola commented Jan 14, 2020

octogonz commented Jan 24, 2020

iclanton commented Jan 29, 2020 • edited Loading

MasterLambaster commented Jan 29, 2020 • edited Loading

iclanton commented Jan 29, 2020

MasterLambaster commented Jan 29, 2020 • edited Loading

iclanton commented Jan 29, 2020

MasterLambaster commented Jan 30, 2020 • edited Loading

iclanton commented Jan 30, 2020

MasterLambaster commented Jan 30, 2020

liyikun commented May 11, 2021

octogonz commented May 13, 2021

D4N14L May 13, 2021

Choose a reason for hiding this comment

D4N14L May 13, 2021

Choose a reason for hiding this comment

octogonz May 14, 2021 • edited Loading

Choose a reason for hiding this comment

octogonz May 14, 2021

Choose a reason for hiding this comment

D4N14L May 13, 2021

Choose a reason for hiding this comment

octogonz May 14, 2021

Choose a reason for hiding this comment

octogonz May 14, 2021

Choose a reason for hiding this comment

octogonz commented May 14, 2021

D4N14L left a comment

Choose a reason for hiding this comment

octogonz commented May 14, 2021

octogonz commented May 15, 2021

msftclas commented Jul 6, 2019 •

edited

Loading

MasterLambaster commented Jul 18, 2019 •

edited

Loading

iclanton commented Jan 29, 2020 •

edited

Loading

MasterLambaster commented Jan 29, 2020 •

edited

Loading

MasterLambaster commented Jan 29, 2020 •

edited

Loading

MasterLambaster commented Jan 30, 2020 •

edited

Loading

octogonz May 14, 2021 •

edited

Loading