smartcar / node-sdk

Smartcar Node.js SDK
MIT License
50 stars 14 forks source link

Fix user agent header and include node.js version #56

Closed gurpreetatwal closed 6 years ago

gurpreetatwal commented 6 years ago

Fixes the user-agent header so that it reports the version of the client making the request as opposed to Smartcar API being accessed (which is in the URL) as it currently does.

I also added in the version of node.js that the request was made from for tracking of which node version we should support. Please take a close look at the format of the header as well as if we should be including more or less information in it. (first glance process.platform might be useful for windows/osx/linux support)

Format: smartcar-node-sdk:<pkg-version> (node.js <node-version>) Example: smartcar-node-sdk:3.0.2 (node.js v6.14.3)

Examples of headers from other API libraries:

KarthikBhaskara commented 6 years ago

@gurpreetatwal Any reason why we're choosing to use : instead of /. Seems like other api's are using /. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent#Syntax

gurpreetatwal commented 6 years ago

Thoughts on:

`Smartcar/${pkg.version} (${process.platform}; ${process.arch}) Node.js ${process.version} `
'Smartcar/4.0.1 (Linux; x64) Node.js 6.14.3'
sankethkatta commented 6 years ago
/^Smartcar\/(\d+\.\d+\.\d+) \((\w+); (\w+)\) Node.js (\d+\.\d+\.\d+)$/
gurpreetatwal commented 6 years ago
/^Smartcar\/(\d+\.\d+\.\d+-[\w-]*) \((\w+); (\w+)\) Node.js (v\d+\.\d+\.\d+)$/

need to include any specifiers after version like 3.0.0-rc1

sankethkatta commented 6 years ago

@gurpreetatwal do you need the specifiers for the process version as well?

emrebsonmez commented 6 years ago

lgtm, just address @sankethkatta's remarks :rocket:

emrebsonmez commented 6 years ago

mr. realtime ;)

gurpreetatwal commented 6 years ago

@sankethkatta at least for Node, I don't think so