Move dependencies we reexpose in our API to `peerDependencies` from `dependencies` by prprabhu-ms · Pull Request #1294 · Azure/communication-ui-library

prprabhu-ms · 2022-01-11T17:58:41Z

What

Move @azure/communication-calling and @azure/communication-chat to peer dependencies for @azure/communication-react and associated packlets.

Why

Because we expose critical classes from these dependencies as part of our own API, they should be peer dependencies -- our client is expected to also depend on these packages in order to be able to use our API, so having private installs of the dependencies can be dangerous....
.... and this breaks @azure/communication-calling as expected. Two installs of the package sometimes don't play well together. They definitely have caused problems when two different versions get installed due to different dependency versions.

How Tested

In a test app, played around with the different cases:

This table shows where @azure/communication-calling is installed, for different cases of how the package.json for communication-react and the client app respectively specify the dependency.

Package.json setup Communication-react / App	Installed in node_modules/?	Installed in node_modules/communication-react/node_modules/	Remarks
dependency/	✅	❌	Current intended behavior
dependency/matching-dependency	✅	❌	Current intended behavior
dependency/non-matching-dependency	✅	✅	❌ The two versions are mismatched. Likely to cause runtime errors
peerDependency/	❌	❌	❌ Breaks the build
peerDependency/matching-dependency	✅	❌	Happy path!
peerDependency/non-matching-dependency	✅	❌	❌ Warning about wrong version, but easy to miss in the console log

Is this a breaking change?

No. This official SemVer FAQ states that patch updates of packages are allowed to require clients to update transitive dependencies.

We shouldn't do this for no reason, to avoid headaches in CI etc, but there is a legitimate reason for this change -- currently it is very easy for clients to get into non-functioning states if the @azure/communication-calling dependency happens to mismatch between their app and our library.

This reverts commit bbf1014.

…ncies-for-exposed-api

github-actions · 2022-01-11T18:03:56Z

Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

github-actions · 2022-01-11T18:04:15Z

Storybook URL https://60c7ae6891f0e90039d7cd54-uwhgoapnbp.chromatic.com/

github-actions · 2022-01-11T18:04:19Z

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

github-actions · 2022-01-11T18:05:51Z

Storybook URL https://60c7ae6891f0e90039d7cd54-frilstjzhw.chromatic.com/

github-actions · 2022-01-11T18:06:11Z

Failed to pass the component examples UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

github-actions · 2022-01-11T18:07:57Z

Storybook URL https://60c7ae6891f0e90039d7cd54-fdqklaayta.chromatic.com/

github-actions · 2022-01-11T18:21:43Z

Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot.

JamesBurnside · 2022-01-11T19:59:43Z

    "@fluentui/react-hooks": "~8.3.8"
  },
  "peerDependencies": {
+    "@azure/communication-calling": "1.3.2",


if we do this, presumably when someone does an npm install they also have to do npm i @azure/communication-calling @azure/communication-chat? If so we'd need to update documentation along side this (which will be weird if we have one set of documentation for both stable and beta)

Given we limit to a single version it's odd to have a peer dependency of a fixed version (although I do see the need). React hooks can detect if there's multiple versions of react and throws a big error - could that be a solution instead?

If only we didn't ask our clients to use --legacy-peer-deps :( (I'm looking at your react-northstar).

Without this flag, the default NPM behavior since v7 is to auto-install peer-dependencies so that they wouldn't necessarily have to install our peer dependencies explicitly.

That said, peer dependencies are the way to go here:

We do plan to move to a minimum-supported-version for these dependencies. At least allow patch version updates for chat/calling without needing to release a new communication-react stable version.

Detecting multiple versions of a package and complaining -- if this is the path we take, it'd have to be the package that dislikes multiple versions (i.e., calling) that does that. Also, it feels wrong to solve this problem at runtime in the package, when NPM dependency management is already intended to solve it.

Perhaps @azure/communication-calling should detect multiple versions and complain, especially if they decide it's a BAD thing to happen. I'd say that it should be in addition to proper dependency management though.

Can we have these allow minor versions today?

Suggested change

"@azure/communication-calling": "1.3.2",

"@azure/communication-calling": ">=1.3.0 <1.4.0",

(I did file an issue with n* to support v17+ microsoft/fluentui#21082)

(perhaps we could even hoist or bundle our last remaining N* component out of the fluent repo: https://github.com/microsoft/fluentui/tree/master/packages/fluentui/react-northstar/src/components/Chat)

Replying to the suggestion for this PR: I don't want to change the actual version supported in any way in this PR (just move the single pinned version to peer deps).

We can start supporting minor versions in the future, but let's keep that a separate PR / discussion. The main concern there is knowing what our support plan is -- folks might start using latest calling before we've integrated it / tested with our own stable release.

JamesBurnside · 2022-01-11T20:01:51Z

Love the table in the PR description - super useful!

github-actions · 2022-01-13T00:22:38Z

Storybook URL https://60c7ae6891f0e90039d7cd54-motcvysfsb.chromatic.com/

github-actions · 2022-01-13T17:28:07Z

Storybook URL https://60c7ae6891f0e90039d7cd54-ygkhtsutvu.chromatic.com/

prprabhu-ms added 6 commits January 7, 2022 21:48

Add boilerplate for custom styled component example

bbf1014

Move @azure/communication-calling to peer dependency

25e109f

Move @azure/communication-chat to peer dependency

84d9fec

Revert "Add boilerplate for custom styled component example"

4cd0a43

This reverts commit bbf1014.

Merge remote-tracking branch 'origin/main' into prprabhu/peer-depende…

cf2b37e

…ncies-for-exposed-api

Change files

d05433a

prprabhu-ms changed the title ~~Prprabhu/peer dependencies for exposed api~~ Move dependencies we reexpose in our API to peerDependencies from dependencies Jan 11, 2022

prprabhu-ms marked this pull request as ready for review January 11, 2022 19:36

prprabhu-ms requested review from JamesBurnside, PorterNan, alkwa-msft, anjulgarg, carocao-msft, dmceachernmsft, edwardlee-msft, mgamis-msft and prabhjot-msft as code owners January 11, 2022 19:36

JamesBurnside reviewed Jan 11, 2022

View reviewed changes

JamesBurnside approved these changes Jan 12, 2022

View reviewed changes

JamesBurnside mentioned this pull request Jan 12, 2022

Add flavor(variants) for stable/beta #1291

Merged

3 tasks

Merge branch 'main' into prprabhu/peer-dependencies-for-exposed-api

1ee30e9

alkwa-msft approved these changes Jan 13, 2022

View reviewed changes

Merge branch 'main' into prprabhu/peer-dependencies-for-exposed-api

3ee7ea7

prprabhu-ms enabled auto-merge (squash) January 13, 2022 17:22

prprabhu-ms merged commit f9e922f into main Jan 13, 2022

prprabhu-ms deleted the prprabhu/peer-dependencies-for-exposed-api branch January 13, 2022 17:30

	"@azure/communication-calling": "1.3.2",
	"@azure/communication-calling": ">=1.3.0 <1.4.0",

Conversation

prprabhu-ms commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How Tested

Is this a breaking change?

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 11, 2022

Uh oh!

JamesBurnside Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prprabhu-ms Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

JamesBurnside Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

JamesBurnside Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

JamesBurnside Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prprabhu-ms Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

JamesBurnside commented Jan 11, 2022

Uh oh!

github-actions Bot commented Jan 13, 2022

Uh oh!

github-actions Bot commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prprabhu-ms commented Jan 11, 2022 •

edited

Loading

JamesBurnside Jan 11, 2022 •

edited

Loading

JamesBurnside Jan 12, 2022 •

edited

Loading