Skip to content

Remove the subscriptions-transport-ws peer dependency#235

Merged
jaydenseric merged 1 commit intojaydenseric:masterfrom
PowerKiKi:master
Nov 8, 2020
Merged

Remove the subscriptions-transport-ws peer dependency#235
jaydenseric merged 1 commit intojaydenseric:masterfrom
PowerKiKi:master

Conversation

@PowerKiKi
Copy link
Copy Markdown
Contributor

@PowerKiKi PowerKiKi commented Nov 5, 2020

Because @apollo/client is declared as a hard dependencies, and it
itself declare its own dependencies, there is no need to match them here,
since we don't reference any of those packages in our code.

jaydenseric
jaydenseric reviewed Nov 5, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@jaydenseric jaydenseric left a comment

Choose a reason for hiding this comment

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

I totally agree, and your PR is how I originally had it. The thing is, I had users complain that Yarn “Berry” PnP strict mode apparently required the peer dependencies to be declared in this package, see:

What do you think about it? I don't use Yarn, so I'm not sure if the situation has changed and it's safe to remove the extra peer dependencies again.

@PowerKiKi
Copy link
Copy Markdown
Contributor Author

PowerKiKi commented Nov 6, 2020

Unfortunately this still happens with yarn 2.3.3. I was not aware of this behavior, but was able to reproduce it by publishing myself a test version of this lib and running:

rm -rf /tmp/test-peer && mkdir /tmp/test-peer && cd /tmp/test-peer && yarn set version berry && yarn --version && yarn init && yarn add apollo-upload-client-test-without-peer @apollo/client graphql

This would lead to the following output:

Resolving berry to a url...
Downloading https://github.com/yarnpkg/berry/raw/master/packages/berry-cli/bin/berry.js...
Saving it into /tmp/test-peer/.yarn/releases/yarn-berry.cjs...
Updating /tmp/test-peer/.yarnrc.yml...
Done!
2.3.3
{
  name: 'test-peer'
}
➤ YN0000: ┌ Resolution step
➤ YN0002: │ apollo-upload-client-test-without-peer@npm:14.1.2 doesn't provide graphql@^14.0.0 || ^15.0.0 requested by @apollo/client@npm:3.2.5
➤ YN0000: └ Completed in 2s 425ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ regenerator-runtime@npm:0.13.7 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ symbol-observable@npm:2.0.3 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ ts-invariant@npm:0.4.4 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ tslib@npm:1.14.1 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ zen-observable@npm:0.8.15 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: Done with warnings in 2s 688ms

Where we can see that yarn complains about graphql. This is clearly documented in https://yarnpkg.com/advanced/error-codes#yn0002---missing_peer_dependency, and matching the peer in this package is indeed one of the solution. That sound like huge burden on lib maintainers...

Anyway, since apollo 3.2.1 subscriptions-transport-ws is also optional. So the only peer we need to match is graphql itself.

I updated my PR accordingly.

@jaydenseric
Copy link
Copy Markdown
Owner

jaydenseric commented Nov 6, 2020

Should we also mark subscriptions-transport-ws as optional, instead of removing it? Have you tested Yarn installs ok without subscriptions-transport-ws in peer dependencies?

@PowerKiKi
Copy link
Copy Markdown
Contributor Author

PowerKiKi commented Nov 6, 2020

There is no need to mention subscriptions-transport-ws at all in our package, because it already is marked as optional in apollo. This variant is the second solution suggested in the docs.

I also tested it out with my test package. Right now if you run the command mentionned in
#235 (comment), you'll get the latest version of my test package which is exactly this PR.

This would get you this correct output:

Resolving berry to a url...
Downloading https://github.com/yarnpkg/berry/raw/master/packages/berry-cli/bin/berry.js...
Saving it into /tmp/test-peer/.yarn/releases/yarn-berry.cjs...
Updating /tmp/test-peer/.yarnrc.yml...
Done!
2.3.3
{
  name: 'test-peer'
}
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed in 0s 730ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ regenerator-runtime@npm:0.13.7 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ symbol-observable@npm:2.0.3 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ ts-invariant@npm:0.4.4 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ tslib@npm:1.14.1 can't be found in the cache and will be fetched from the remote registry
➤ YN0013: │ zen-observable@npm:0.8.15 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed
➤ YN0000: Done in 0s 970ms

Another proof that it is not necessary is the fact that react also is an optional peer in @apollo/client. But it was never mentioned in our package.json. And it never caused any issue.

jaydenseric
jaydenseric requested changes Nov 6, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@jaydenseric jaydenseric left a comment

Choose a reason for hiding this comment

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

We need to add a changelog.md entry that includes this PR link.

@PowerKiKi
Copy link
Copy Markdown
Contributor Author

PowerKiKi commented Nov 6, 2020

done in a squashed commit

@PowerKiKi @jaydenseric
Remove the subscriptions-transport-ws peer dependency
ae8c26d 
It can be safely removed because it’s not used by this package and it’s now an optional @apollo/client peer dependency.
jaydenseric
jaydenseric approved these changes Nov 8, 2020
View reviewed changes
Copy link
Copy Markdown
Owner

@jaydenseric jaydenseric left a comment

Choose a reason for hiding this comment

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

Thanks for your work :)

@jaydenseric jaydenseric changed the title Remove unnecessary peerDependencies Remove the subscriptions-transport-ws peer dependency Nov 8, 2020
@jaydenseric jaydenseric merged commit dbfba84 into jaydenseric:master Nov 8, 2020
@jaydenseric
Copy link
Copy Markdown
Owner

jaydenseric commented Nov 8, 2020

Published in apollo-upload-client@14.1.3 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaydenseric jaydenseric jaydenseric approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PowerKiKi @jaydenseric