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.

Sign up for GitHub

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

RFC: Use ranges to define versions of dependencies for converged components #17499

Merged
merged 1 commit into from
May 12, 2021
Merged

RFC: Use ranges to define versions of dependencies for converged components #17499

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions rfcs/convergence/dependency-versions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# RFC: Use ranges to define versions of dependencies

@layershifter @ling1726 @miroslavstastny

## Summary

This RFC proposes to continue use caret (`^`) and tilde (`~`) for production dependencies of converged packages.

## Problem statement

The are two options to manage packages:

- ranges via carets (`^1.0.0`), tildes (`~1.0.0`) or signs (`>=1.0.0`, `1.0.0 || 2.0.0`)
- exact versions

The preferred approach may vary on use case: applications and libraries follow different practices. Fluent UI is a library, so we should focus on the best experience for customers.

Usage of exact versions in our dependencies solves a problem with different versions for us and customers so there will be a guarantee that they will use same versions as we. This should guarantee us there will be no unknown bugs caused by third party dependencies in different versions. This is a huge benefit.

However, an approach with exact versions creates a different set of problems for consumers. Let's assume that we have two packages and ours will use an exact version:

```json
{
"name": "@fluentui/react-COMPONENT",
"dependencies": {
"lodash": "4.17.20"
}
}
```

```json
{
"name": "eslint",
"dependencies": {
"lodash": "^4.17.21"
}
}
```

This will result in two versions of installed packages by a package manager (`yarn` in this case):

```
lodash@4.17.20:
version "4.17.20"
resolved "https://..."
integrity sha512-...

lodash@^4.17.21:
version "4.17.21"
resolved "https://..."
integrity sha512-...
```

Thus a single package gets included multiple times in a Webpack bundle due to different package versions. This situation will happen without any warning, resulting in extra bloat in consumer's bundle and may lead to hard-to-find bugs.

It's also harder for consumers in this case to consume security patches, for example `lodash@4.17.20` contains [a security vulnerability](https://snyk.io/vuln/SNYK-JS-LODASH-1018905) but this version will be inside consumer's application without additional movements from their side. If we will use a caret ("lodash": "^4.17.20") only a single version (a latest in a range) will be installed:
Copy link
Member

Choose a reason for hiding this comment

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

If we will use a caret ("lodash": "^4.17.20") only a single version (a latest in a range) will be installed

Just a heads up, this is not necessarily true--if a dep on lodash@^4.17.20 resolved to 4.17.20 already exists in the lockfile, and that semver spec is still referenced by another package, yarn may add a separate entry for lodash@^4.17.21 => 4.17.21 instead of resolving both to 4.17.21.

(Doesn't affect the proposal since this is an issue with yarn in general, but I wanted to call it out for awareness.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're totally right 👍 But with ranges we can at least deduplicate dependencies manually or via tools (yarn-deduplicate for Yarn v1 or yarn dedupe for V2).


```
lodash@^4.17.20, lodash@^4.17.21:
version "4.17.21"
resolved "https://..."
integrity sha512-...
```

There are workaround that customers can use in this case ([`yarn resolutions`](https://classic.yarnpkg.com/en/docs/selective-version-resolutions/), [webpack aliases](https://webpack.js.org/configuration/resolve/#resolvealias)) to use a proper version. But we should not create an additional walls for customers to properly consumer the library.

There is a Webpack plugin (similar to [duplicate-package-checker-webpack-plugin](https://github.com/darrenscerri/duplicate-package-checker-webpack-plugin)) that prevents imports of duplicate versions on Teams side.

## Detailed Design or Proposal

Use semver ranges to define production dependencies for our customers. Exact versions can be used in non-distributed code (build and infra tools).
Copy link
Member

Choose a reason for hiding this comment

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

A couple possible issues/nuances here that I wonder if we should include in the proposal re: different versions for production and dev dependencies:

  1. How do we set this up with syncpack? My first idea would be something like this (supposing converged1 and converged2 depend on lodash@^4.17.21, whereas other packages have a specific version in devDependencies):
{
  "packages": ["@fluentui/converged1", "@fluentui/converged2"],
  "dependencies": ["lodash"]
}
  1. How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo? Without tooling enforcement, it's entirely possible that yarn could choose to resolve the two versions differently. My first idea (which probably won't work, see below) is this. (The leading ** is necessary to make resolutions work for packages within the workspace.)
{
  "resolutions": {
    "**/@fluentui/*/lodash": "4.17.21"
  }
}

However this might not work because the resolutions spec says a single * is not supported (would probably be good to test it in case the implementation is different). **/@fluentui/**/lodash should be supported, but that would resolve all nested tools' lodash deps to a specific version, which might be a problem. So we'd probably have to figure out some other approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we set this up with syncpack?

Do you mean our repo, correct? Can please expand a bit more there, I did not get the problem.

How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo?

IMO we should not guarantee this, for example: we don't restrict people to use the same version of React as in our repo, why it should be different for other dependencies? It means that we should be accurate in choosing dependencies, they should follow semver.

However, resolutions can be and should be used by customers for legal issues. I tried your approach with resolutions on Yarn v1:

{
  "dependencies": {
    "@fluentui/react-components": "^9.0.0-alpha.36",
    "@popperjs/core": "^2.9.2"
  },
  "resolutions": {
    "**/@fluentui/*/@popperjs/core": "2.8.0"
  }
}

This results in two versions installed:

"@popperjs/core@2.8.0", "@popperjs/core@~2.4.3":
  version "2.8.0"

"@popperjs/core@^2.9.2":
  version "2.9.2"

Copy link
Member

Choose a reason for hiding this comment

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

How do we set this up with syncpack?

Do you mean our repo, correct? Can please expand a bit more there, I did not get the problem.

With syncpack you're only allowed to depend on one semver spec for a given dep, unless you set up a versionGroups entry allowing certain packages to use a different version spec for that dep.

How do we guarantee that yarn resolves the production dep to the same version as the dev dep within our repo?

IMO we should not guarantee this, for example: we don't restrict people to use the same version of React as in our repo, why it should be different for other dependencies? It means that we should be accurate in choosing dependencies, they should follow semver.

I meant within our repo, not from a customer standpoint. Yarn may sometimes decide to resolve foo@^1.0.0 (production dep) and foo@1.1.0 (dev dep) differently, resulting in duplicates within our build process, which is a significant part of the problem @Hotell was trying to resolve to begin with.

The resolutions suggestion was also meant to be used within our repo. (If it doesn't work as I wrote it, we could do some more experimenting.)

Copy link
Member Author

@layershifter layershifter May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@ecraig12345 so this thread is related only to our repo correct?

With syncpack you're only allowed to depend on one semver spec for a given dep, unless you set up a versionGroups entry allowing certain packages to use a different version spec for that dep.

As I know @Hotell proposed to move out all dev dependencies to the root package.json. If we will do this and ignore the root package.json in syncpack, will it solve the issue?

Yarn may sometimes decide to resolve foo@^1.0.0 (production dep) and foo@1.1.0 (dev dep) differently, resulting in duplicates within our build process, which is a significant part of the problem @Hotell was trying to resolve to begin with.

Honestly I don't think that I understand what solves usage of exact versions for dev dependencies.

  • we are sure that we are using an exact version, for example webpack@5.0.0
  • it does not solve duplicate dependencies as storybook may depend on webpack@^5.1.0
  • this results in two entries for webpack (and potentially its sub dependencies) in our yarn.lock

Are these statements correct?


_Optionally_: Distribute with packages `VERSIONS` file that will contain versions of dependencies that are used on our side in machine readable format that can be potentially used by customers in their pipelines.
Copy link
Member

Choose a reason for hiding this comment

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

Let's not be too hasty, as I said will follow up with OSS CELA on the actual requirements ASAP

Copy link
Member

Choose a reason for hiding this comment

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

CELA conclusion: we can only run enhanced license scan on specific versions we use, so this idea could work.

Copy link
Member

Choose a reason for hiding this comment

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

By "this idea" do you mean the VERSIONS file? How would that work from a consumer's perspective, specifically re: legal? (Feel free to respond on teams if needed.)

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial idea was that we can implement a Webpack plugin that will ensure that included dependencies match versions defined in VERSIONS file.


## Pros and Cons

<!-- Enumerate the pros and cons of the proposal. Make sure to think about and be clear on the cons or drawbacks of this propsoal. If there are multiple proposals include this for each. -->

## Discarded Solutions

We potentially can use `peerDependencies` like React does, however these is no much sense in it: React does it to intentionally avoid multiple versions (it was not possible before React 17, [facebook/react#1939](https://github.com/facebook/react/issues/1939)). However, it creates issues for customers as they will need to install our dependencies, too:

```bash
$ yarn add @fluentui/react-COMPONENT DEPENDENCY1 DEPENDENCY2 ... DEPENDENCY999
```

_[NPMv7](https://blog.npmjs.org/post/631877012766785536/release-v700) installs `peerDependencies` by default._

## Open Issues

NA