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

Multiple @accounts/client versions: consider using peerDependencies #33

Open
diegonc opened this issue Mar 6, 2017 · 8 comments
Open

Comments

@diegonc
Copy link

diegonc commented Mar 6, 2017

If you follow the installation procedure stated in the README, there will be multiple versions of @accounts/client installed resulting in the following error when loading the page:

Uncaught (in promise) Error: no tokens provided
    at AccountsError.ExtendableBuiltin (http://192.168.56.101:3000/bundle.js:78029:29)
    at new AccountsError (http://192.168.56.101:3000/bundle.js:78059:117)
    at AccountsClient._callee7$ (http://192.168.56.101:3000/bundle.js:60416:24)
    at tryCatch (http://192.168.56.101:3000/bundle.js:17026:41)
    at Generator.invoke [as _invoke] (http://192.168.56.101:3000/bundle.js:17261:23)
    at Generator.prototype.(anonymous function) [as next] (http://192.168.56.101:3000/bundle.js:17078:22)
    at step (http://192.168.56.101:3000/bundle.js:60100:192)
    at http://192.168.56.101:3000/bundle.js:60100:362

You can reproduce it by starting a project from scratch, which as of today, leads to the following packages being installed:

yarn list v0.21.3
├─ @accounts/[email protected]
│  ├─ @accounts/common@^0.0.7
│  ├─ @accounts/[email protected]
│  │  └─ lodash@^4.16.4
│  ├─ immutable@^3.8.1
│  ├─ jwt-decode@^2.1.0
│  ├─ lodash@^4.16.4
│  ├─ redux-immutable@^3.0.8
│  └─ redux@^3.6.0
├─ @accounts/[email protected]
│  ├─ apollo-errors@^1.2.1
│  └─ lodash@^4.16.4
├─ @accounts/[email protected]
│  ├─ @accounts/[email protected]
│  ├─ @accounts/[email protected]

Also, in js-accounts/examples you can remove client package from package.json and do

yarn add @accounts/client

now when you run the example the cited exception should appear.

This error is produced by the fact that multiple versions of @accounts/client introduce different instances of the AccountClient class being used by the several modules that require it.

By switching to using peer dependencies the user of react and react-material-ui can upgrade other components of js-accounts independently as long as versions are compatible.

Does this make sense?


EDIT: I revisited this issue (because of trying out graphql support) and realised that it was not commons the culprit but @accounts/client.

@diegonc diegonc changed the title Consider using peerDependencies Multiple @accounts/common versions: consider using peerDependencies Mar 6, 2017
@TimMikeladze
Copy link
Member

Yes it does. Do you mind sending PR?

@TimMikeladze
Copy link
Member

Actually, no need for the PR, I'll handle this in mine.

@diegonc diegonc changed the title Multiple @accounts/common versions: consider using peerDependencies Multiple @accounts/client versions: consider using peerDependencies Mar 8, 2017
@diegonc
Copy link
Author

diegonc commented Mar 8, 2017

I don't know if GitHub notifies edits, so ping 😛

Just found out that only one version of @accounts/client must be in existence, commons is not to blame here. I updated the issue body and title.

@TimMikeladze
Copy link
Member

Github edits don't notify. I wish they did (as well as reactions).

Thanks.

@TimMikeladze
Copy link
Member

Material ui should also be marked as a peer dependency.

@diegonc
Copy link
Author

diegonc commented Mar 9, 2017

I'm not so sure about that one.
It's more like a one-of kind of dependency; you can have material, but also bootstrap, unstyled or whatever UI framework people implements.

@TimMikeladze
Copy link
Member

Here is my reasoning: If the consuming code is using the @accounts/react-material-ui package then they already have the material-ui in their package.json. It should be also added as a dev dependency here so the storybook would work too.

@diegonc
Copy link
Author

diegonc commented Mar 9, 2017

Oops, sorry, I thought you were referring to react-material-ui. Now it's crystal clear 😄

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

No branches or pull requests

2 participants