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

Allow the client initialisation to be deferred optionally #377

Closed
nlisgo opened this issue May 5, 2017 · 9 comments
Closed

Allow the client initialisation to be deferred optionally #377

nlisgo opened this issue May 5, 2017 · 9 comments
Assignees

Comments

@nlisgo
Copy link
Contributor

nlisgo commented May 5, 2017

Steps to reproduce

  1. If the window.hypothesisConfig function is available but the config that you return needs to be prepared by performing an ajax call. If the embed.js hypothes.is client has already been initialised then it will discount the values that are trying to be retrieved by ajax.

Expected behaviour

Tell us what should happen.
Allow the initialisation of the hypothes.is client to be delayed optionally and then the developer who includes the client embed.js can programmatically trigger the client initialisation once the configuration has been retrieved by ajax.

Actual behaviour

The client is being rendered by a grantToken is available so the user does not appear to be logged in on the hypothes.is client.

Browser/system information

chrome but this should be browser agnostic.
one.

@ajpeddakotla
Copy link

@nlisgo there's some discussion that's need around this - @sean-roberts is going to post some questions here on this in a bit.

@sean-roberts
Copy link
Contributor

sean-roberts commented May 18, 2017

from research on hypothesis/product-backlog#66
these two options were suggested to support asynchronous grant token usage. The sidebar would recognize that we are in that state and will stay logged out until that value is passed.

JS callback - expose a token update function (from a window.hypothesis namespace) that the integrator can invoke when they receive a new token (it's passed the token directly)

  • caveat to this one is that the integrator would need to make sure our code is loaded before invoking (typically not a huge problem for people)

Register Promise - expose a function that the integrator can invoke with a promise that will eventually resolve a token

  • similar to the callback approach except it allows a way to tell us that we are receiving on but will not worry about getting the actual value until the promise resolves.
    like the callback approach, the integrator would need to wait for our code to load and expose the hook.

Of these approaches, I would suggest that we go with the promise approach. Perhaps off of the hypothesisConfig function. Something like this:

window.hypothesisConfig = function(){
  return {
    services: [{
      grantToken: new Promise(function(resolve, reject){
      	// ...do my async thing
        resolve(grantToken);
      })
    }]
  };
};

What we need is a way for an integrator to tell us the value when they get the value. Using promises is just the standard way to pass a value that will be given in the future (that is to say, doing the callback approach might be a reinvention of promise).

In the example, grantToken is overloaded to accept a string or a promise value without introducing another field and worrying about which one we prioritize higher if they are both provided. Though, I am totally open to exploring that.

Note that we already have mechanisms to unload and reload the user's state based on authentication changes. Once we are given the value we will piggy back on those functions to reload the sidebar

@robertknight
Copy link
Member

Notes from backlog refinement discussion (23/5/17). Nick noted that what eLife have actually implemented is an endpoint which returns the grant token. What we could do is to have the client provide the URL of a cookie-authenticated endpoint which returns a fresh grant token. The sidebar application would then be responsible for fetching this token. The next step is to talk to @nlisgo at eLife to clarify if this would be more convenient for them than the proposal above.

@ajpeddakotla
Copy link

@nlisgo can you provide your feedback on the discussion above? Thanks!

@nlisgo
Copy link
Contributor Author

nlisgo commented Jun 6, 2017

We could certainly cope with the workflow that @robertknight has suggested. I am not in a position to comment which approach would be easier for general adoption of the publisher group functionality. I support your recommendation to register a promise for the retrieval of the grantToken.

@robertknight
Copy link
Member

One question that isn't addressed in the discussion above is why we don't simply rely on the publisher to dynamically add the client boot script to their page, once they have fetched the grant token. This is pretty easy to do in code and I believe it is what @nlisgo did in the prototype.

The one benefit I can see is that the client can be initialised concurrently with the grant token being fetched so annotations should appear faster. Is there any other reason I missed?

@robertknight
Copy link
Member

IIRC an alternative proposal that was made is that instead of providing us with a token directly, the integrator would instead provide us with the URL of an endpoint that returns the token. The client would then be responsible for making a request (with cookies) to that URL in order to fetch the token.

The upside is that for publishers like eLife, this might mean less work as they could just provide the URL of an endpoint that they need to create anyway. A downside is that the only mechanism that this endpoint can use for authentication is cookies which might be an issue if the client is used inside say an SPA.

@robertknight
Copy link
Member

@nlisgo for context, can you remind me why it was in your setup that you needed to fetch the token after the page loads?

@seanh
Copy link
Contributor

seanh commented Jun 22, 2017

Closing this in favor of hypothesis/product-backlog#330, @nlisgo I think that the proposed solution in #330 should work for you, but if you could comment on hypothesis/product-backlog#330 and lets us know if the proposed solution works for you, that'd be great.

@seanh seanh closed this as completed Jun 22, 2017
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

5 participants