MobilityData / gtfs-realtime-bindings

Language bindings generated from the GTFS Realtime protocol buffer spec for popular languages.
Apache License 2.0
373 stars 129 forks source link

docs: improved JS example #104

Closed jameslinjl closed 1 year ago

jameslinjl commented 1 year ago

A couple of small improvements for the JS example:

gauthier-th commented 1 year ago

I think we should use the Fetch API (with the node-fetch implementation for Node.js) instead of https. This would make this example more universal since the Fetch API is also used in web browsers. Moreover, the Fetch API is coming to Node.js v18 as an experimental feature, so it is expected to become stable in future releases.

jameslinjl commented 1 year ago

Thanks for your thoughts @gauthier-th !

This would make this example more universal since the Fetch API is also used in web browsers

I'm not sure if we would want this particular example to be extended to web browsers? The network request to read a GTFS realtime feed would almost definitely require an API token, which we would want to discourage developers (especially more junior ones, who may be more likely to use this repo) from exposing on the frontend client. Providing an example that works in the browser may lead some newer developers to accidentally expose their API credentials on the web.

the Fetch API is coming to Node.js v18 as an experimental feature

My general preference is to stick with currently stable features for documentation, so my gut says that fetch still being experimental in node.js is probably a reason to hold off on using it in a documentation example.

Let me know your thoughts on these!

gauthier-th commented 1 year ago

I'm not sure if we would want this particular example to be extended to web browsers?

I was not thinking only about the web in particular, but also other environments in JavaScript using the Fetch API, but not the HTTPS API of Node.js (for instance I can think about React Native and Deno in addition to web)

The network request to read a GTFS realtime feed would almost definitely require an API token

Concerning the API token, I'm not sure this always relevant. For example, in France, the government publishes many GTFS-RT datasets in open access (from this I can access the real-time transit of my town).

we would want to discourage developers (especially more junior ones, who may be more likely to use this repo) from exposing on the frontend client

I don't really agree with you on this. We should give a less relevant example so that junior developers don't make the mistake of putting their API token into production?

My general preference is to stick with currently stable features for documentation, so my gut says that fetch still being experimental in node.js is probably a reason to hold off on using it in a documentation example.

That's why I proposed to use node-fetch, which is a widely used fetch implementation for Node.js (35M downloads per month)

jameslinjl commented 1 year ago

@gauthier-th Your thoughts make sense to me! I'm working on some other stuff the rest of today and probably won't revisit this repo until later this week or sometime next week, so feel free to push some proposals onto this PR and make a separate one using node-fetch

gauthier-th commented 1 year ago

@jameslinjl It's done in #107, feel free to give me your feedback 😉