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

feat: Support the workspace: protocol #2450

Closed
wants to merge 2 commits into from

Conversation

nicolo-ribaudo
Copy link
Contributor

Description

Yarn 2 and pnpm (I know that it isn't supported, but saw that you are working on it) support the workspace: protocol for monorepo package versions.

When used, it explicitly marks a dependency to assert that it is never downloaded from the registry. Both the package managers allow making the workspace: protocol required: if it isn't specified, then the dependency is downloaded from the registry even if a local package would match the requested version.
yarn docs, pnpm docs.

This PR contains two different commits: the first one makes lerna not fail when someone uses the workspace protocol (which is what made me open this PR).

The second one implements a --local-dependencies option which makes lerna actually use the workspace protocol. I'm not sure that this is needed since people would probably directly enable the useWorkspaces option and delegate this task to yarn, but I added the commit just in case you could be interested in it. I would have no problems with dropping it.

Please don't review them as a single unit 🙏

Motivation and Context

I didn't open an issue about it, but this is currently blocking Babel from upgrading to yarn 2 (babel/babel#11142).

How Has This Been Tested?

I added tests to the affected packages.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (I will do it depending on what gets accepted)
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am super-pumped for this functionality, I'm so sorry I ignored it for so long!

My chief concern is that we're not covering the version/publish scenario, which is pretty important for this protocol to be "fully" supported by Lerna.

@@ -273,6 +274,37 @@ describe("Package", () => {
);
});
});

describe(".updateLocalDependency()", () => {
it("works with workspace: protocols", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have any direct tests of this method here because it is repeatedly tested in the publish and version tests. This is good to have, though.

expect(pkg.toJSON()).toMatchInlineSnapshot(`
Object {
"dependencies": Object {
"a": "workspace:^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One wrinkle, however, is that sometimes pkg.updateLocalDependency() is intended to overwrite any protocol (such as file:) with the resolved version. Existing code has cheesy conditionals that exclude the call when the resolved.type === "directory", so we're gonna have to make this method smarter somehow (or split the intent into two methods...)

@@ -209,6 +209,9 @@ class Package {
if (resolved.registry || resolved.type === "directory") {
// a version (1.2.3) OR range (^1.2.3) OR directory (file:../foo-pkg)
depCollection[depName] = `${savePrefix}${depVersion}`;
if (resolved.explicitWorkspace) {
depCollection[depName] = `workspace:${depCollection[depName]}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the newly-added test for this method, this isn't always the case (thinking specifically of prepublish resolution of ${protocol}: versions)...

* @param {Boolean} [opts.rejectCycles] Whether or not to reject cycles
* @constructor
*/
constructor(packages, opts) {
const options = QueryGraphConfig(opts);

// Create dependency graph
this.graph = new PackageGraph(packages, options.graphType);
this.graph = new PackageGraph(packages, options.graphType, options.localDependencies);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run-topologically also needs this option passed through, as unfortunately most of its callsites are passing custom config, not the whole command-resolved options object...

{
"commands": {
"link": {
"localDependencies": "explicit"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing CLI option config for --local-dependencies, as well as README documentation. It's especially burdensome for an option like this because it is such a broad thing. Perhaps instead of a flag, we need to centralize the "am i using a custom protocol" logic better?

  • // resolve relative file: links to their actual version range
    const updatesWithLocalLinks = this.updates.filter(node =>
    Array.from(node.localDependencies.values()).some(resolved => resolved.type === "directory")
    );
  • if (depVersion && resolved.type !== "directory") {
    // don't overwrite local file: specifiers, they only change during publish
    pkg.updateLocalDependency(resolved, depVersion, this.savePrefix);
  • rootHasLocalFileDependencies() {
    const rootDependencies = Object.assign({}, this.project.manifest.dependencies);
    return Object.keys(rootDependencies).some(
    name =>
    this.targetGraph.has(name) &&
    npa.resolve(name, rootDependencies[name], this.project.rootPath).type === "directory"
    );
    }

After all, in an ideal world, the workspace: protocol would "just work", similar to how file:/link: is handled today.

@ChiefORZ
Copy link

why isn't this feature getting any more attention? we are waiting hard for this one...

@nicolo-ribaudo
Copy link
Contributor Author

@ChiefORZ I won't have time to work on this PR for 1-2 months. If you want to take my PR and address the review comments it would be appreciated 🙏

@arcanis
Copy link

arcanis commented Sep 25, 2020

@evocateur if by any chance you saw a way to land this PR in any way I'd appreciate - it's currently difficult for third-party projects to use the workspace: protocol without having to drop Lerna, which makes the adoption ever so slightly harder for us 🙁

@Carl-Foster
Copy link

I'm happy to take over this PR and address the comments.

@Carl-Foster
Copy link

I've put some time into this PR and trying to add tests to address the comments however I'm finding it hard to navigate. I feel like lerna needs to detect if yarn berry is being used and transparently pass through most of the logic down to the @yarnpkg/core package. This would require a fairly significant rework of the implementation and I don't know enough about the architecture to make that decision.

@evocateur evocateur changed the base branch from master to main November 8, 2020 00:40
@okikio
Copy link

okikio commented Dec 20, 2020

Can someone please merge this pull request, I have been waiting for this feature for literally 9 months.... It looks like all the details concerning this have been ironed out, so, please merge it.

@nicolo-ribaudo
Copy link
Contributor Author

@okikio I don't have time to work on this PR anymore, but @evocateur already reviewed it (#2450 (review)).
If you want to cherry-pick my commits, handle @evocateur's review and open a new PR, that would probably help a lot moving this forward.

@okikio
Copy link

okikio commented Dec 20, 2020

@okikio I don't have time to work on this PR anymore, but @evocateur already reviewed it (#2450 (review)).
If you want to cherry-pick my commits, handle @evocateur's review and open a new PR, that would probably help a lot moving this forward.

I thought the PR was finalized and just not yet merged. Ok, I'll open a new PR but how do I get @evocateur to review.

@okikio
Copy link

okikio commented Dec 26, 2020

@nicolo-ribaudo I have gone through your commits, I don't think they would adequately accomplish the task required. When discussing the workspace protocol, most devs using lerna would most likely want it to act like if the workspaces protocol wasn't used. By default, lerna can't set packages to use the workspace protocol, so why bother building it into lerna, why not just let users continue with the tools they were using prior, so, for example, yarn and pnpm, if devs are already using these tools it would be less work to implement, all that would be required would be to just add an option to ignore the workspace protocol and let either yarn or pnpm handle the publishing, so, lerna would handle versioning and changelog generation. What do you think @evocateur does this sound good?

const explicitWorkspace = /^workspace:/.test(spec);
if (explicitWorkspace) {
spec = spec.replace(/^workspace:/, "");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a sidenote: The same should be done for the :patch protocol.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one can't really be replaced

@evocateur
Copy link
Member

I've rebased this onto the latest in main: https://github.com/lerna/lerna/compare/test/workspace-protocol

I'm going to do some local validation, maybe write some more tests. We're still lacking option config for --local-dependencies (and documentation), so this still isn't ready to go as-is.

I can't promise any timeline, but this is increasing in priority for me.

@brianespinosa
Copy link

For anyone else still needing this, it seems like the right approach might just be implementing publishing features yourself on top of yarn as they are doing in the above referenced PR from Storybook.

@ghiscoding
Copy link

ghiscoding commented May 11, 2022

hey everyone happy to share that workspace: protocol is now supported in Lerna-Lite with this new release >=1.3.0.
Give it a try and happy coding 🎉

charlie-yao referenced this pull request in charlie-yao/react-aria-widgets Oct 5, 2022
@fahslaj
Copy link
Contributor

fahslaj commented Dec 29, 2022

To all it may concern, this PR is being closed for clarity since Lerna already supports the workspace: protocol. It was added a while ago as part of the pnpm support included in Lerna 6.

Lerna recognizes the workspace: prefix in Yarn 1, Yarn 2+ (with node_modules linker) and pnpm workspaces and correctly converts them to the appropriate dependency spec when publishing.

Thank you to everyone for your participation in the Lerna community!

@fahslaj fahslaj closed this Dec 29, 2022
@binaryunary
Copy link

Does this mean #2564 can be closed?

@JamesHenry
Copy link
Member

Thanks for the reminder @binaryunary, yes!

@cumt-robin
Copy link

Couldn't find any versions for "@jview/utils" that matches "workspace:*"

I dont know why...

it's lerna v7.

image

@JamesHenry
Copy link
Member

@cumt-robin You are using yarn v1/classic which never supported the workspace protocol. If you want to use it you migrate to latest yarn v3+

@cumt-robin
Copy link

cumt-robin commented Sep 11, 2023 via email

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

Successfully merging this pull request may close these issues.

None yet