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

Feature request: Query params #17

Closed
ashtuchkin opened this issue Dec 21, 2015 · 9 comments
Closed

Feature request: Query params #17

ashtuchkin opened this issue Dec 21, 2015 · 9 comments

Comments

@ashtuchkin
Copy link

Nice library, thanks for creating it!

What do you think about adding some convenience features?

Url query params

connect((props) => ({
  data: {
    url: "/api/data",
    query: { from: props.from, limit: props.limit }  // => "/api/data?from=xxx&limit=yyy
  }
}))(Component)
@hnordt
Copy link
Contributor

hnordt commented Dec 21, 2015

I like both ideas. 👍

Just one suggestion, instead data:

connect(({ name }) => ({
  barFetch: {
    url: `/foos/${name}`,
    query: { foo: verifyFoo(), bar: verifyBar() }
    transform: ({ foos, page }) => ({ bars: foos, page  }),
    then: (foo) => `/bar-for-foos-by-id/${foo.id}`
  }
}))

@ashtuchkin
Copy link
Author

That's even better! :)

@hnordt
Copy link
Contributor

hnordt commented Dec 21, 2015

@ryanbrainard,

If you approve the syntax I'll be happy to create a pull request.

@ryanbrainard
Copy link
Contributor

To do the data transform, you should be able to use then() on the PromiseState instead of on the request in connect(). The latter will fire off a new request, which is probably not what you want.

@ryanbrainard
Copy link
Contributor

@hnordt do you mean call the 'query' attribute 'data' (and maybe also share it with 'body') like jquery does? I wasn't following your example in relation to the issue.

@ryanbrainard
Copy link
Contributor

Also, can we please break this into two separate issues? :)

@hnordt
Copy link
Contributor

hnordt commented Dec 23, 2015

@ryanbrainard I think transforming data on PromiseState is ok, better than add a new prop.

About query. Sorry, I miss typed the verify part. Another example:

connect(({ name }) => ({
  barFetch: {
    url: `/foos/${name}`,
    query: { foo: 'bar', bar: 'foo' }
  }
}))

// barFetch should request /foos/${name}?foo=bar&bar=foo

I think objects are better than raw string for query params because we can do things like this:

{ query: Object.assign({ foo: 'bar' }, { bar: testBar() ? 'foo' : null }) }

@ashtuchkin
Copy link
Author

Moved discussion of transform functions to #21

@ashtuchkin ashtuchkin changed the title Query params, transform functions Feature request: Query params Dec 23, 2015
@ryanbrainard
Copy link
Contributor

I've been thinking about this, and I've decided to decline on this feature for the following reasons:

  1. The query is part of the URL. Having it be in a separate attribute can lead to confusion.
  2. It requires the use of request object syntax. The URL string request syntax should always be preferred for simplicity.
  3. I agree that declaring the query params as an object/hash is good (I do this myself), but I think that converting it to a query string is something that should be done by an explicit function. We could provide something like this in this library, but I'm sure there are several existing libraries that already provide such a facility (e.g. jQuery's $.param()), so I don't think there would be much value in implementing another one. ES6 string templates/interpolation make this even nicer to use to include in a string.

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

3 participants