firesphere

If the tests are green and @tractorcow thinks it's wrapped up now, I should merge it and tag a new version indeed

firesphere

Time things. Wibbly wobbly timey wimey things prevented me from mergirg

nils

here: https://github.com/Firesphere/silverstripe-graphql-jwt/pull/24

Show 1 attachment(s)
tractorcow

https://github.com/Firesphere|@Firesphere would love to discuss this with you before merging; See you on slack maybe?

Overview of changes in general order of most significant to least

• Return type of all validate / create mutations has been consolidated and normalised. The standard response is now a MemberToken type with these fields: • Member (the member) • Token (string token) • Valid (bool valid flag) • Status (enum status code of the token) • Code (int HTTP code that matches the status) • Message (string message of humanised status code) • Multiple authenticator tokens per user; Allows multiple devices to have their own tokens. • Anonymous authentication is now shifted up into a custom Authenticator class; Support injectable authenticators into createToken mutation. Removed anonymous_allowed • When logging in anonymously, create a real member object to better integrate with silverstripe security logging / auditing practices. • Bump minimum dependency to php 7.1: strict_types are everywhere too. • Updated tests and documentation of course • Support multiple schemas (graphql 3.0 and above). • Relative path supported for JWT tokens • Updated renewal process to support separate token / refresh expiration dates • I gave every file my Love. Green ticks everywhere.

https://github.com/Firesphere|@Firesphere if you merge, I suggest branching master -> 1 first, as this is a new 2.x major change.

Hide attachment content
MichalKleiner

@Bjorn Bojumble relevant gh https://github.com/silverstripe/silverstripe-graphql/issues/181

Show 1 attachment(s)
NightJar

I have recently run into a use case where mid-development of a feature I made a pull from the upstream repository.
Unbeknownst to me at the time some GraphQL fields had been added to a query, however being that this was defined in yaml things were destined not to work until a flush had taken place.

As I was not aware I simply loaded the CMS without a flush... and nothing happened.

Literally nothing. Half of it loaded (HTML templates), but everything depending on a GraphQL query as the source of data just hung with a loading screen. There was no hint of an error besides a great lack of data. My console was quiet of errors, every XHR response was 200 OK, and there were no UI updates to notify me of an issue.

Luckily a colleague informed me of the alteration, so I was quickly back on track, but if he had not been available at the time I would have been left to laboriously run through each XHR and check the output matched my expectations manually.

To this I see a few different approaches to solving this problem:

HTTP response codes
As I had asked the server for something it didn't know about, the problem lies with me and my request. I feel like there should be something in the 4xx range (like 422) that is a far more appropriate response.

UI Update
However with rendering a 4xx response this resolves with most handlers as an error.

This could stop our UI from updating in the bad case... an argument against HTTP error codes - in which I make a counter case we should still update it to let the user (developer - I'm in dev mode) know.

In a good case we could still render what data we did receive (if any) with a caveat that some is potentially missing.

In any case with the admin module I think there's still plenty of space to render a global toast style notification that an issue has happened with our data request no matter the UI or section that is open at the time.

tl;dr there needs to be at least something to let a developer know that things have gone awry.
Something more than a hung screen and a message buried deep within an "everything's fine" response.

https://user-images.githubusercontent.com/778003/44966302-8700e600-af8e-11e8-9933-42fc0a17763a.png|image

Hide attachment content
😂 (1)