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 send additional data via meta key #56

Closed
wants to merge 3 commits into from

Conversation

7rulnik
Copy link

@7rulnik 7rulnik commented May 3, 2017

@relekang @mxstbr
It should resolve #24

But I think that opportunity to pass something more complicated than strings will be cool. How we should pass arrays or objects?

@7rulnik 7rulnik changed the title Master Allow send additional data via meta key May 3, 2017
@codecov-io
Copy link

codecov-io commented May 3, 2017

Codecov Report

Merging #56 into master will decrease coverage by 0.65%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   71.13%   70.47%   -0.66%     
==========================================
  Files           6        6              
  Lines          97      105       +8     
  Branches       20       21       +1     
==========================================
+ Hits           69       74       +5     
  Misses         28       28              
- Partials        0        3       +3
Impacted Files Coverage Δ
src/handler.js 72.5% <100%> (-0.48%) ⬇️
src/utils.js 90.9% <100%> (+2.67%) ⬆️
src/index.js 0% <0%> (-7.7%) ⬇️
src/db.js 77.77% <0%> (-5.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2add293...8439008. Read the comment docs.

Copy link
Member

@relekang relekang left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@relekang
Copy link
Member

relekang commented May 4, 2017

But I think that opportunity to pass something more complicated than strings will be cool. How we should pass arrays or objects?

I am kind of thorn between this would be nice to have and it is a good thing to have the restriction.
What do you think @mxstbr? We can add it later under another param name e.g. meta_json.

If we want to support more complex data structures I think urlencoded json is a good way to go.

Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

LGTM, let's ship this for now and then discuss about more complex stuff in an issue?

@7rulnik
Copy link
Author

7rulnik commented May 4, 2017

Honestly I don't think that ?meta=k1:v1,k2:v2 is a good way for passing additional data.

  1. User should generate this string
  2. Maximum lenght of url — 2000 symbols
  3. Support meta and meta_json will be pain. 2 keys that do same things.
  4. We need additional logic for parsing ?meta=k1:v1,k2:v2. It's not really hard, but it can be easier.

I think that POST is correct way for passing meta.

fetch('/users', {
  method: 'POST',
  headers: {
    'Content-Type': 'application/json'
  },
  body: JSON.stringify({
    meta: {},
  })
})

And JSON.parse on server.

I prefer to decide how it will be before shipping something for users.

@relekang
Copy link
Member

relekang commented May 4, 2017

I agree, I am thinking that it would be great if we turned it so that get only reads and post is the way to put things into the db.

@7rulnik
Copy link
Author

7rulnik commented May 4, 2017

If we split get and put it would be major release, so for now we can merge this PR and release it as minor version. After this I can start splitting.

@mxstbr
Copy link
Member

mxstbr commented May 5, 2017

it would be great if we turned it so that get only reads

I'm not a fan of that, I quite like that GET also increments because it supports this pattern: (which is what I built micro-analytics for initially)

window.onload = () => {
  fetch(`my-analytics.com/${window.location.href}`)
    .then(data => console.log(`${data.views} views`)) 
}

I do agree that ?meta isn't the best choice though, we could just do POST to add meta data but GET still increments?

@relekang
Copy link
Member

relekang commented May 5, 2017

I do agree that ?meta isn't the best choice though, we could just do POST to add meta data but GET still increments?

👍

@relekang
Copy link
Member

Closing this as we want to revisit without meta in GET requests. The meta-branch contains a wip version of that on top of the lerna restructure.

@relekang relekang closed this Jun 18, 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

Successfully merging this pull request may close these issues.

?meta query param
4 participants