feat: modernizing our JS and Node snippet targets #245
Conversation
erunion
added
enhancement
erunion
changed the title
There was a problem hiding this comment.
These fixtures extension got moved from .cjs to .js because this repository is ESM and import can't be used in .cjs files.
There was a problem hiding this comment.
This, and the JS fetch client, still diverge from each other because this newly revised Node fetch client needs to actually be able to support file uploads which short of importing fs into JS fetch snippets can't be otherwise done.
It makes me wonder how much value we really get from these plain JS snippet generation now that Node has converged with JS entirely on the fetch front. Axios on Node and JS work the same, do we really need two entirely separate target systems, with identical clients, for the same language? How many people are still out here looking for jQuery $.ajax() snippets when fetch is available? If we did put an fs into JS snippets for multipart file uploads would that be a problem?
Should we drop javascript as an available target in favor of reframing the node target as Node + JS?
There was a problem hiding this comment.
cool with whatever you roll with, but i’m generally of the mindset that we shouldn’t unilaterally remove any libraries for all customers unless they’re explicitly deprecated and/or insecure. as for combining the two targets, i'm open to doing that further down the line but don't think that should happen in this PR. if folks complain that we dropped XHR as a result of this PR, then that's a good indicator that we should keep the node + JS targets separate.
There was a problem hiding this comment.
i'm definitely in favor of removing node-fetch, request, and unirest but i'm a little hesitant to remove xhr and native for the reasoning i mentioned here, but i'm cool with moving forward with this and backfilling clients if folks complain. a few non-blocking suggestions below but otherwise lgtm
There was a problem hiding this comment.
cool with whatever you roll with, but i’m generally of the mindset that we shouldn’t unilaterally remove any libraries for all customers unless they’re explicitly deprecated and/or insecure. as for combining the two targets, i'm open to doing that further down the line but don't think that should happen in this PR. if folks complain that we dropped XHR as a result of this PR, then that's a good indicator that we should keep the node + JS targets separate.
This reverts commit 281a09c.
🧰 Changes
JS
fetchresponsetores.jqueryresponsetores.Node
axiosaxiosimport fromrequiretoimport.URLSearchParamsfromnode:urlas its global now.responsetoresanderrortoerr.fetchnode-fetchto nativefetch.error:output header from logged errors because it's already usingconsole.error()..requestunirest