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

Various internal auth and client changes #14

Closed
wants to merge 13 commits into from
Closed

Conversation

chrissnyder
Copy link
Contributor

@rogerhutchings and @edpaget would like different pairs of eyes on this to see if I'm moving in the right direction. Not ready to merge yet.

Now that a number of different custom projects are using the client, here are a number of refactorings that I believe supports custom development a bit better. In no particular order:

  • Takes heavy inspiration from https://github.com/zooniverse-ui work on testing and switches to using mocha/chai as a test framework. Not a strict improvement, but I'd personally prefer a consistent test environment for all our JS code.
  • PanoptesClient now extends JSONAPIClient. So no more client.api to actually access the client.
  • Remove the checkCurrent() user promise and falls back to a simple evented system. client.auth is an instance of EventEmitter. To catch auth changes, just listen for change on client.auth
  • Isomorphic. Should work fine in both node and the browser.
  • Removed all compiled code from the repo. I'm not 100% sure of the ramifications of this.
  • Updated auth code to sync back up with PFE.

@@ -9,77 +12,33 @@ const DEFAULT_OPTS = {
'Accept': 'application/vnd.api+json; version=1'
},
host: 'https://panoptes.zooniverse.org',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to change this to zooniverse.org?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will do

@edpaget
Copy link
Contributor

edpaget commented Aug 17, 2015

How does it work serverside. I think the json-api-client uses XMLHttpRequest which doesn't exist in node as far as I know? We could use something like superagent?

@rogerhutchings
Copy link
Contributor

It doesn't, and yeah, @brian-c mentioned using superagent in the client at one point as well. I've been polyfilling it with xmlhttprequest and xmlhttprequest-cookie

@chrissnyder
Copy link
Contributor Author

it uses https://github.com/chrissnyder/node-xmlhttprequest-cookie stubbed at https://github.com/zooniverse/panoptes-javascript-client/blob/refactor/src/client.js#L1-L4 to make XMLHttpRequest work in Node. Basically wraps http.request to give it the same API as xmlhttprequest.

Better solution might be to use superagent within json-api-client rather than relying on this polyfill, but I took these changes independent of any changes to json-api-client.

@edpaget
Copy link
Contributor

edpaget commented Aug 17, 2015

I can't build this. There's no src/index.js file which is being used in the gulp tasks.

@edpaget
Copy link
Contributor

edpaget commented Aug 17, 2015

Ah @chrissnyder you're only installing the XHR polyfill as a devDependency, so it won't be around if you include in a different project.

@chrissnyder
Copy link
Contributor Author

Can now npm run build and moved xmlhttprequest-cookie to just a straight dependency.

@chrissnyder chrissnyder changed the title Towards refactor Various internal auth and client changes Aug 17, 2015
@chrissnyder
Copy link
Contributor Author

@brian-c Been meaning to talk with you about this, but keep getting pulled away on other things. Would still like your thoughts on the direction this is going and maybe it'd be best to put them here so others can chime in as well.

@brian-c
Copy link
Contributor

brian-c commented Aug 19, 2015

Yeah, my only strong suggestions are to use superagent (like you're already discussing) and start using this in the PFE ASAP.

When v2 of the API is ready we should drop JSON-API-Client for a generic adapter we don't have to maintain. That's going to be a nightmare amount of work, so something to start thinking about.

Nice work!

@edpaget
Copy link
Contributor

edpaget commented Aug 19, 2015

I just put in a pull request to add support for using superagent to the json-api-client lib: https://github.com/zooniverse/json-api-client/pulls

@chrissnyder
Copy link
Contributor Author

Updated the PR to use Ed's superagent version of JAC. Also did some serious housecleaning of the gulp tasks and dependencies.

@chrissnyder
Copy link
Contributor Author

@edpaget With https://github.com/zooniverse/json-api-client being merged in, I think this PR is also good to go.

@rogerhutchings
Copy link
Contributor

Is this ready to go?

@chrissnyder
Copy link
Contributor Author

Will double check this afternoon and report back.

@rogerhutchings rogerhutchings mentioned this pull request Dec 17, 2015
@brian-c
Copy link
Contributor

brian-c commented Jan 27, 2016

Replaced by #20.

@brian-c brian-c closed this Jan 27, 2016
@mcbouslog mcbouslog deleted the refactor branch November 5, 2018 20:09
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.

4 participants