micro-analytics / micro-analytics-cli

Public analytics as a Node.js microservice. No sysadmin experience required! πŸ“ˆ
MIT License
734 stars 39 forks source link

Allow send additional data via meta key #56

Closed 7rulnik closed 7 years ago

7rulnik commented 7 years ago

@relekang @mxstbr It should resolve https://github.com/micro-analytics/micro-analytics-cli/issues/24

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

codecov-io commented 7 years ago

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%) :arrow_down:
src/utils.js 90.9% <100%> (+2.67%) :arrow_up:
src/index.js 0% <0%> (-7.7%) :arrow_down:
src/db.js 77.77% <0%> (-5.56%) :arrow_down:

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.

relekang commented 7 years ago

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.

7rulnik commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.