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

Replace fetch api #2309

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Replace fetch api #2309

wants to merge 5 commits into from

Conversation

YuMuuu
Copy link

@YuMuuu YuMuuu commented Nov 27, 2023

Use fetch api and remove simple-get depedency (context: #2223 (comment)).


  • Have you updated CHANGELOG.md?

@YuMuuu
Copy link
Author

YuMuuu commented Nov 27, 2023

I will make sure that the relevant PR is made 👀
I rebase if necessary or split PR.

@YuMuuu YuMuuu changed the title Use fetch api Replace fetch api Nov 27, 2023
@zbjornson
Copy link
Collaborator

Thanks, that's a nice change. #2310 will need to land first to drop support for older versions of Node.js.

@YuMuuu
Copy link
Author

YuMuuu commented Nov 28, 2023

Thanks, that's a nice change. #2310 will need to land first to drop support for older versions of Node.js.

Ok. I can rebaes after merged #2310

@YuMuuu
Copy link
Author

YuMuuu commented Nov 28, 2023

c0725b8

This commit is important, Should I separate PR?

lib/image.js Outdated

get.concat({
url: val,
const request = fetch(url = val, data = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This creates two new global variables, url and data. I'm guessing it was a misstake

Suggested change
const request = fetch(url = val, data = {
const request = fetch(val, {

lib/image.js Outdated
get.concat({
url: val,
const request = fetch(url = val, data = {
method: "POST",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use single quotes to align with the rest of the file?

Suggested change
method: "POST",
method: 'POST',

lib/image.js Outdated
Comment on lines 49 to 51
})

request.then((res) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since request is only used once, you could chain here and remove the variable:

Suggested change
})
request.then((res) => {
})
.then((res) => {

lib/image.js Outdated
return res.arrayBuffer()
})
.then((data) => {
return setSource(this, data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

setSource doesn't return anything?

Suggested change
return setSource(this, data)
setSource(this, data)

lib/image.js Outdated
return setSource(this, data)
})
.catch((err) => {
return onerror(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Suggested change
return onerror(err)
onerror(err)

Also, the behaviour of onerror is that it throws if a handler hasn't been set. Before this change, that thrown was delegated to the global error handler.

Now it will instead be eaten by this promise, which I don't think is desirable.

I think that util.callbackify or process.nextTick would be best used to restore the old behaviour 🤔

Copy link
Author

Choose a reason for hiding this comment

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

=> 93cfd2f

I'm not sure if the changes I made are correct...

@LinusU
Copy link
Collaborator

LinusU commented Nov 29, 2023

@YuMuuu could you rebase on latest master? I've merged #2310 and #2313 which are alternative ways of dealing with the problems you had with dtslint and minimum version of Node...

@YuMuuu
Copy link
Author

YuMuuu commented Nov 29, 2023

  • rebase master and drop unnecessary commit

lib/image.js Outdated
util
.callbackify(
fetch(val, {
method: 'POST',
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if the changes I made are correct.

simple-get.concat method type is GET.

@YuMuuu YuMuuu force-pushed the use-fetch-api branch 2 times, most recently from 5213c1f to a2e3845 Compare November 29, 2023 19:01
@YuMuuu
Copy link
Author

YuMuuu commented Dec 3, 2023

@LinusU I fixed it

@YuMuuu
Copy link
Author

YuMuuu commented Dec 6, 2023

@LinusU This is just a reminder if you missed my commit. Could you please take a look at my commits I’ve done last week and merge it if it looks ok.

@YuMuuu
Copy link
Author

YuMuuu commented Apr 30, 2024

@LinusU Could you please review this PR? ;;

@YuMuuu
Copy link
Author

YuMuuu commented May 24, 2024

@zbjornson Could you review, as it seems that @LinusU is busy?

@kunipon
Copy link

kunipon commented Sep 11, 2024

@LinusU Could you please let me know when the pull request is expected to be merged?

@LinusU
Copy link
Collaborator

LinusU commented Sep 11, 2024

Sorry for being a bit slow on the feedback here...

Looking at the code, I don't think that it works? Have you tested this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants