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

UserSession API + Storage Strategies #542

Open
wants to merge 33 commits into
base: develop
from

Conversation

9 participants
@larrysalibra
Copy link
Member

larrysalibra commented Sep 10, 2018

This pull request moves user-session related calls into the instance of an object UserSession. Data related to a given user-session is encapsulated in this instance and persisted by an instance of a object that extends SessionDataStore.

Defaults such as app domain, etc continue to be auto-detected when run in a browser environment.

This means the library can continue to be used with minimal configuration in a web browser environment:

import { UserSession } from 'blockstack'

const userSession = new UserSession()

userSession.redirectToSignIn()

userSession.getFile('file.json')

In a web browser, by default UserSession will store session data in the browser's localStorage through an instance of LocalStorageStore.

This means that app developers can leave management of session state to users.

In a non-web browser environment, it is necessary to pass in an instance of AppConfig which defines the parameters of the current app.

  import { AppConfig, UserSession } from 'blockstack'
  const appConfig = new AppConfig('http://localhost:3000')
  const userSession = new UserSession({ appConfig })

In non-browser environments (ie. node), session state is stored in the instance using the InstanceDataStore. If a developer wishes to store this elsewhere, he should can extend SessionDataStore and implement his own storage strategy.

You can test this by linking it to this branch of the blockstack todos app: https://github.com/blockstack/blockstack-todos/tree/feature/blockstack-19

In the directory of this checked out repo run:

npm link

In the blockstack-todos directory run:

npm link blockstack
npm run start

This pull request requires some additional documentation and test coverage before it is ready to be merged. I wanted to get this in front everyone earlier for review of the proposed API which has been modified slightly from the original proposal.

larrysalibra added some commits Aug 21, 2018

@larrysalibra larrysalibra added this to In progress in 19.0.0 release via automation Sep 10, 2018

@blockstack blockstack deleted a comment from CLAassistant Sep 10, 2018

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 29, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

friedger
kantai
larrysalibra
You have signed the CLA already but the status is still pending? Let us recheck it.

friedger and others added some commits Oct 22, 2018

Merge pull request #554 from friedger/patch-1
Add missing new for error constructor

@larrysalibra larrysalibra changed the title UserSession API + Storage Strategies [WIP] UserSession API + Storage Strategies Nov 24, 2018

@larrysalibra

This comment has been minimized.

Copy link
Member

larrysalibra commented Nov 24, 2018

You can also test this using the animal-kingdom demo app: https://github.com/larrysalibra/animal-kingdom

@benoror benoror referenced this pull request Nov 24, 2018

Open

Implement service for blockstackAuth #11

0 of 2 tasks complete
@kantai

This comment has been minimized.

Copy link
Member

kantai commented Nov 27, 2018

Thanks for this @larrysalibra -- I'll put this on my review and testing plate for this week.

@yknl

This comment has been minimized.

Copy link
Collaborator

yknl commented Dec 4, 2018

Thanks Larry, the changes look good to me.

One small thing is that the initialization of the UserSession is a little bit confusing.

Using the animal kingdom app as an example, in App.js, a user session is initiated with

this.userSession = new UserSession()

In Landing.js and SignedIn.js it is initiated with an app config object

this.userSession = new UserSession({ appConfig })

It's not immediately clear to me why they are different.

Additionally, would it be better to simplify the config parameter to a generic object? This removes the need for an AppConfig class.

For example:

this.userSession = new UserSession({ 
  appConfig: {
    scopes: ['store_write', 'publish_data']
  },
  sessionStore: sessionDataStore
})
@@ -310,7 +310,9 @@ export class BlockstackNetwork {
* @return {Promise} a promise that resolves to the WHOIS-like information
*/
getNameInfo(fullyQualifiedName: string) {
return fetch(`${this.blockstackAPIUrl}/v1/names/${fullyQualifiedName}`)
console.log(this.blockstackAPIUrl)

This comment has been minimized.

@kantai

kantai Dec 4, 2018

Member

this should be a debug statement

This comment has been minimized.

@larrysalibra

larrysalibra Dec 5, 2018

Member

Thanks for catching this! I pushed a fixed.

@@ -321,6 +323,7 @@ export class BlockstackNetwork {
}
})
.then((nameInfo) => {
console.log(`nameInfo: ${JSON.stringify(nameInfo)}`)

This comment has been minimized.

@kantai

kantai Dec 4, 2018

Member

this should also be a debug statement

This comment has been minimized.

@larrysalibra

larrysalibra Dec 5, 2018

Member

Thanks for catching this! I pushed a fixed.

* @returns {string} - URI
*/
redirectURI() : string {
return `${this.appDomain}${this.redirectPath}`

This comment has been minimized.

@aulneau

aulneau Dec 4, 2018

Contributor

Would there ever be a case where we might not want to hardcode the redirectURI as a combo appDomain and redirectPath, eg for native apps or electron apps, I might want to share the data bucket for coinsapp.co but use coins://authRequest as the redirect for the native implementations. cc @kantai @yknl @hstove

This comment has been minimized.

@friedger

friedger Dec 4, 2018

Collaborator

@aulneau custom schemes should not be abused for deep links. What is the specification of coins:// scheme? If another app handles this scheme the authentication will fail because it does not know the transit key.

19.0.0 release automation moved this from In progress to Needs review Dec 4, 2018

@kantai
Copy link
Member

kantai left a comment

These changes look okay to me. There's two questions I have, though:

  1. Why not try to make this somewhat backwards compatible by doing something like setting the UserSession storage strategy on import? That way, we could still have the blockstack.js functions (getFile, putFile, etc.) defined at the top-level, rather than being object methods.

  2. If we're going to make these breaking changes, we should include some kind of documentation like "How do I upgrade my application to blockstack.js v19?" Even with that, there will be a lot of out-of-date documentation following this release: @moxiegirl, are you tracking this?

@moxiegirl

This comment has been minimized.

Copy link
Contributor

moxiegirl commented Dec 4, 2018

@kantai I'm tracking this convo yes. A few points:

  • perhaps we should time this merge? We have a lot releasing pre-holidays and not a lot of folks will be around to help if things go wobbly. What do you think?

  • I don't have scope for coming up with an upgrade steps myself...so someone needs to take a rough pass at those.

  • Do we need to include upgrading all our samples such as hello-world, ios, android, in concert with the merge.

  • Need a formal forum post and something we can use a blog post might be good too

@kantai

This comment has been minimized.

Copy link
Member

kantai commented Dec 4, 2018

Do we need to include upgrading all our samples such as hello-world, ios, android, in concert with the merge.

Probably not for ios and android, but, eventually, yes for the three sample apps on blockstack.org, and the yeoman app-generator, because those will all be instructions that apply to old versions of the library.

Need a formal forum post and something we can use a blog post might be good too

Agreed. A blog post would be ideal, because this is a very nice new functionality, but also a breaking change that our app developers will need to make.

I don't have scope for coming up with an upgrade steps myself...so someone needs to take a rough pass at those.

Yep -- we'll make sure that someone makes a draft of this document, and I think such a draft would be a requirement before merging these changes.

perhaps we should time this merge? We have a lot releasing pre-holidays and not a lot of folks will be around to help if things go wobbly. What do you think?

Maybe -- I think we should enumerate what all would need to be updated, and then see whether or not that's achievable before the holidays.

@hstove

This comment has been minimized.

Copy link
Contributor

hstove commented Dec 4, 2018

I still think this is a pretty heavy breaking change that could be mitigated by my suggestion in the forum:

https://forum.blockstack.org/t/proposal-giving-control-of-session-state-to-app-developers/6006/24

If others (mainly Larry) are ok with this, I can take a stab at the implementation.

Then no one would have to change any code (including our tutorials) and still be able to upgrade.

@yknl

This comment has been minimized.

Copy link
Collaborator

yknl commented Dec 4, 2018

I still think this is a pretty heavy breaking change that could be mitigated by my suggestion in the forum

I would be supportive of keeping the existing API for a period of time, logging a deprecation warning and eventually deprecate them in a later version.

@kantai

This comment has been minimized.

Copy link
Member

kantai commented Dec 4, 2018

I still think this is a pretty heavy breaking change that could be mitigated by my suggestion in the forum

I agree @hstove.

I am open to breaking changes, I just want to hear an argument against trying to make this non-breaking first.

@friedger

This comment has been minimized.

Copy link
Collaborator

friedger commented Dec 4, 2018

Developers need to do the following changes when adopting this update:

  • create a UserSession varable, say blockstackSession
  • replace prefix blockstack with the blockstackSession for
    • redirectToSignIn
    • isSignInPending
    • handlePendingSignIn
    • loadUserData
    • isUserSignedIn
    • signUserOut
    • makeAuthRequest
    • generateAndStoreTransitKey
    • redirectToSignInWithAuthRequest
    • getAuthResponseToken
    • verifyAuthResponse
    • getFile
    • putFile
    • deleteFile
    • encryptContent
    • decryptContent

That should be all.

@moxiegirl

This comment has been minimized.

Copy link
Contributor

moxiegirl commented Dec 4, 2018

@kantai How about updating the app-generator code? Or is that ok as is?

Also, we likely need a little project to track all the work to update the sample apps etc. right? Better than a checklist here.

Like who is going to update the sample application code? I can certainly update the docs but someone needs to check the code in completed projects.

@hstove

This comment has been minimized.

Copy link
Contributor

hstove commented Dec 4, 2018

How about updating the app-generator code? Or is that ok as is?

We'd have to update the app generator(s) too.

@aulneau

This comment has been minimized.

Copy link
Contributor

aulneau commented Dec 5, 2018

Additionally I think we can create a codemod to help with migration: https://github.com/facebook/codemod

A real life example -> https://github.com/styled-components/styled-components-codemods

@larrysalibra

This comment has been minimized.

Copy link
Member

larrysalibra commented Dec 5, 2018

Thanks for all the great feedback everyone! ❤️pull request reviews!

@yknl wrote about UserSession optionally taking an object with an AppConfig

It's not immediately clear to me why they are different.

Additionally, would it be better to simplify the config parameter to a generic object? This removes the need for an AppConfig class.

I agree that the Animal Kingdom example is confusing - the App.js instance should probably take an AppConfig instance to be consistent.

We could certainly use a "generic" object instead of an instance of a class, however, that object would in and of itself be an implicit type that should be defined in the type checker.

@hstove wrote:

I still think this is a pretty heavy breaking change that could be mitigated by my suggestion in the forum:

https://forum.blockstack.org/t/proposal-giving-control-of-session-state-to-app-developers/6006/24

If others (mainly Larry) are ok with this, I can take a stab at the implementation.

I think this is a really great way to approach maintaining backwards compatibility. @zone117x (i think it was Matt) in our engineering meeting also suggested publishing these changes with the API @hstove suggests along with deprecation warnings in a new 18 release.

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Dec 5, 2018

I believe for jsdoc deprecation warnings to be visible to developers in their editors (code completion / intellisense / IDE warnings) we may need to change the npm packaging. I've outlined the issue a bit on the forum. To summarize: many editors/IDEs can't parse the babel shims / modules in lib/*.js.

One solution is to perform only the Flow transform from src/*.js to lib/*.js, then use babel for lib/*.js to dist/blockstack.js.

This also has the benefit of exposing all the nice jsdoc while coding against blockstack.js.

@larrysalibra

This comment has been minimized.

Copy link
Member

larrysalibra commented Dec 5, 2018

@zone117x I really like your suggestion on improving editor code completion and documentation compatibility. That would be a huge improvement in developer experience.

This is probably something that makes sense as a separate issue/pull request - we can certainly decide to not ship a new release until such a PR makes it into develop.

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Dec 5, 2018

This is probably something that makes sense as a separate issue/pull request - we can certainly decide to not ship a new release until such a PR makes it into develop.

I have a lot of this done but its in a Flow to typescript port I experimented with a few weeks ago. I'll check if my non-typescript related package.json and babel changes work. If so I'll PR for further discussion.

@zone117x

This comment has been minimized.

Copy link
Member

zone117x commented Dec 11, 2018

Created a PR that allows support for the deprecation warnings in jsdoc/editor/IDE. The PR may take some time for testing and review so if we don't care very much about the deprecation visibility then might make sense to go ahead with this PR without it.

kantai added some commits Jan 10, 2019

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