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

feat: modernizing our JS and Node snippet targets #245

Merged
merged 22 commits into from
Sep 5, 2024

Conversation

erunion
Copy link
Member

@erunion erunion commented Sep 4, 2024

🧰 Changes

JS

  • 🏄 fetch
    • Is now the default target. 🆕
    • Renamed response to res.
  • 🏄 jquery
    • Renamed response to res.
    • Moved response handling to arrow functions.

Node

  • 🏄 axios
    • Moved the axios import from require to import.
    • No longer importing URLSearchParams from node:url as its global now.
    • Renamed response to res and error to err.
    • Moved response handling to arrow functions.
  • 🏄 fetch
    • Has moved from node-fetch to native fetch.
    • Is now the default target. 🆕
    • Dropped an error: output header from logged errors because it's already using console.error()..
  • 🔪 request
    • It's been deprecated for a couple years now.
  • 🔪 unirest
    • Hasn't been updated in six years, was a bespoke HTTP client from the Mashape/Kong folks and never reached an adoption level worth us supporting on all customer sites.

@erunion erunion added enhancement New feature or request refactor Issues about tackling technical debt labels Sep 4, 2024
@erunion erunion changed the title feat: modernizing some snippet target feat: modernizing our JS and Node snippet targets Sep 5, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

These fixtures extension got moved from .cjs to .js because this repository is ESM and import can't be used in .cjs files.

Copy link
Member Author

@erunion erunion Sep 5, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

@erunion erunion marked this pull request as ready for review September 5, 2024 04:52
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

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

src/targets/node/fetch/client.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/targets/node/fetch/client.ts Show resolved Hide resolved
src/targets/node/fetch/client.ts Show resolved Hide resolved
@erunion erunion requested a review from kanadgupta September 5, 2024 18:23
@erunion erunion merged commit 281a09c into main Sep 5, 2024
12 checks passed
@erunion erunion deleted the feat/snippet-modernization branch September 5, 2024 18:33
kanadgupta added a commit that referenced this pull request Sep 23, 2024
This was referenced Sep 23, 2024
kanadgupta added a commit that referenced this pull request Sep 23, 2024
This reverts commit 281a09c aka #245 so
we can re-publish it as a breaking change.
kanadgupta added a commit that referenced this pull request Sep 23, 2024
kanadgupta added a commit that referenced this pull request Sep 23, 2024
This reverts commit c65e264 (#247) and
re-adds the changes from #245, this time as a breaking change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants