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

generatePublicKey promise and generateAppId state #29

Merged
merged 1 commit into from
Dec 8, 2014

Conversation

joscha
Copy link
Collaborator

@joscha joscha commented Nov 30, 2014

  • add hint to generatePublicKey method about callback parameter (it now returns a promise and does not take a callback any more as it used to)
  • generateAppId should take param in favor to local state

@thom4parisot
Copy link
Owner

👍

Should we remove the internal publicKey variable? It can be generated at anytime now, and synchronously so it does not matter much to store it in the current instance of crx.

What do you think?

@joscha
Copy link
Collaborator Author

joscha commented Dec 7, 2014

@oncletom yes, I think removing state is a good idea - wasn't sure if there was some reason to keep it except backwards compat.

@thom4parisot
Copy link
Owner

Nah it's better to clean that up and to reflect that through another major bump version again, when all the other PRs get merged as well.

@joscha
Copy link
Collaborator Author

joscha commented Dec 7, 2014

Okay, so how about I rebase this PR and we open a second one to completely remove the this.publicKey state?

@thom4parisot thom4parisot modified the milestone: 3.0.0 Dec 7, 2014
* add hint to generatePublicKey method about callback parameter
@thom4parisot
Copy link
Owner

I've seen you have rebased that's good!

Okay for an extra PR to remove the publicKey from the internal state as well 👍

thom4parisot pushed a commit that referenced this pull request Dec 8, 2014
generatePublicKey promise and generateAppId state
@thom4parisot thom4parisot merged commit 91814d9 into thom4parisot:master Dec 8, 2014
@joscha joscha deleted the generateAppId-fixes branch December 8, 2014 09:58
@joscha
Copy link
Collaborator Author

joscha commented Dec 8, 2014

👍

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.

2 participants