-
Notifications
You must be signed in to change notification settings - Fork 397
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
Hit the search API #93
Conversation
I haven't tested the isomorphic stuff and the API needs better test coverage. Wanted to get the PR up though. |
This is ready for review. I'm going to make this universal in mozilla/addons#5660. |
import { Schema, arrayOf, normalize } from 'normalizr'; | ||
|
||
// FIXME: Load this from something. | ||
const API_HOST = 'https://addons-dev.allizom.org'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed https://github.com/mozilla/addons-frontend/issues/105 to support client-side configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you load this in via config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was giving me undefined
. That file uses process.env
throughout so I don't know how it would work in the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right - duh - ok we should deal with that separately with a flag instead e.g: https://github.com/erikras/react-redux-universal-hot-example/blob/master/webpack/dev.config.js#L107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So config.get('env')
is "production"
in the browser (through karma anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried getting this to work but it didn't work on the first try and I'm not sure if we'll just need process.env.NODE_ENV
or more than that so I'm going to leave it for the other issue.
06fa034
to
97e08dc
Compare
.withArgs({query: 'Yahoo!', entities: {addons: {foo: {}, bar: {}}}, result: ['foo', 'bar']}) | ||
.once() | ||
.returns(loadAction); | ||
handleSearch('Yahoo!', response).then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this returns a promise if you just simply return it you won't need to use done
and failure should get handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I had tried that originally but I must not have been returning the promise from my helper function so it just blew up. That's much cleaner.
r+wc |
30197d4
to
fa130e4
Compare
const addon = new Schema('addons', {idAttribute: 'slug'}); | ||
|
||
function makeQueryString(opts) { | ||
// FIXME: This should use a real query string generator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This adds normalizr for helping with API responses.
fa130e4
to
1d6806b
Compare
This adds normalizr for helping with API responses. Fixes mozilla/addons#5618.