PanoptesJS: remove superagent #6299
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shaunanoordin
added
the
experiment
Comment on lines
+56
to
+73
| /* | ||
| Converts a query object (e.g. { foo: 'bar', img: 'http://foo.bar/baz.jpg' }) | ||
| into a query string (e.g. "foo=bar&img=http%3A%2F%2Ffoo.bar%") | ||
| */ | ||
| function getQueryString (query, endpoint = '') { | ||
| const queryParams = getQueryParams(query) | ||
| let queryString = Object.entries(queryParams) | ||
| .filter(([key, val]) => val !== undefined) | ||
| .map(([key, val]) => `${encodeURIComponent(key)}=${encodeURIComponent(val)}`) | ||
| .join('&') | ||
|
|
||
| if (queryString.length > 0) { | ||
| return endpoint.includes('?') | ||
| ? `&${queryString}` // If endpoint already has a query '?', prepend the query string with a '&'. | ||
| : `?${queryString}` // Otherwise, prepend with a '?'. | ||
| } | ||
| return '' | ||
| } |
There was a problem hiding this comment.
You can use URLSearchParams to convert an object to a query string in all browsers and in Node too.
There was a problem hiding this comment.
const searchParams = new URLSearchParams(queryParams)
return searchParams.toString()
|
Might be a good idea to upgrade the code to ES6 too. At the moment, it’s written for old versions of Node that are no longer supported. |
|
|
||
| return request.query(queryParams).then(response => response) | ||
| const queryString = getQueryString(query, endpoint) | ||
| const fetchHeaders = Object.assign({ |
There was a problem hiding this comment.
Object.assign has been replaced by the object spread operator, which is much faster, in modern JavaScript.
There was a problem hiding this comment.
Object.assign should only appear in code if you need to support IE.
Comment on lines
+82
to
+85
| const fetchHeaders = Object.assign({ | ||
| 'Content-Type': 'application/json', | ||
| 'Accept': 'application/vnd.api+json; version=1', | ||
| }, parseHeaders(headers)) |
There was a problem hiding this comment.
Suggested change
| const fetchHeaders = Object.assign({ | |
| 'Content-Type': 'application/json', | |
| 'Accept': 'application/vnd.api+json; version=1', | |
| }, parseHeaders(headers)) | |
| const fetchHeaders = { | |
| 'Content-Type': 'application/json', | |
| 'Accept': 'application/vnd.api+json; version=1', | |
| ...parseHeaders(headers) | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Overview
Package: panoptes-js
This PR attempts to remove
superagentfrom the PanoptesJS clientsuperagentis a dependency that we don't technically need anymore, since the standardfetchAPI works fine, while maintaining superagent can be a bit of a faff.panoptes.js, so replacing the dependency should be very easy.Dev Notes
As of 12 Sep 2024, this PR is in an experimental stage. The biggest lessons we've learnt are:
superagent()calls inpanoptes.js, as initially observed.panoptes.get()/panoptes.put()/panoptes.post()/panoptes.del(),Comparison of the default response data we get from superagent vs the default response data we get from fetch().
Superagent response:
Fetch API response:
So in practice, we either need to...
Status
Experimental. Put on pause as of 12 Sep 2024 while higher priority items in PanoptesJS is addressed.
As of commit 96096e: panoptes.get experimental: replace superagent with fetch, the get() function has superagent() successfully replaced with fetch(), with a very rudimentary fetch-response-to-superagent-response transformer in place. You can run
lib-classifier > yarn devand load a local test project, and the data will be fetched fine for this instance.