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 outdated http request dependency with Node 18's fetch #7589

Open
3 tasks done
dblythy opened this issue Sep 22, 2021 · 20 comments
Open
3 tasks done

Replace outdated http request dependency with Node 18's fetch #7589

dblythy opened this issue Sep 22, 2021 · 20 comments
Labels
type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@dblythy
Copy link
Member

dblythy commented Sep 22, 2021

New Feature / Enhancement Checklist

Current Limitation

Our docs state:

You can use your favorite npm module to make HTTP requests, such as request. Parse Server also supports Parse.Cloud.httpRequest for legacy reasons

Why should we continue to support a legacy HTTP module, which hasn't been updated in 3 years, where there are more popular solutions that constantly update their security issues and vulnerabilities?

The http module also uses deprecated methods, such as querystring and parse.

Also request is archived, so we shouldn't recommend that either.

We've seen Parse.Cloud.httpRequest be the source of many issues

Feature / Enhancement Description

Migrate to a dedicated HTTP library, such as axios

Example Use Case

Log Parse.Cloud.httpRequest is depreciated. Please migrate to different HTTP request solution

Alternatives / Workarounds

  • Keep the legacy library as is.
  • Set Parse.Cloud.httpRequest as an alias to axios.

3rd Party References

@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 22, 2021

Thanks for opening this issue!

  • 🎉 We are excited about your ideas for improvement!

@dblythy
Copy link
Member Author

dblythy commented Sep 22, 2021

Related: #6361, #7232

@mtrezza
Copy link
Member

mtrezza commented Sep 22, 2021

Do we even need to offer Parse.Cloud.httpRequest? Any dev can (and many maybe already do) choose their own library depending on its capabilities. What is the benefit for devs in having this method? This seems to be a legacy convenience method, because some years back it was more complex to make a simple http request, but today there are libs available that abstract the complexity away.

@dblythy
Copy link
Member Author

dblythy commented Sep 22, 2021

I would say there is no real reason why we should offer it. Perhaps we should remove it entirely from V5 as a breaking change?

@mtrezza
Copy link
Member

mtrezza commented Sep 22, 2021

Maybe we could deprecate the method in v5 and remove it entirely in v6? If the method has security issues, then we could think about investing some time into using axios during the deprecation.

@mtrezza mtrezza added 🧬 enhancement type:bug Impaired feature or lacking behavior that is likely assumed and removed 🧬 enhancement labels Sep 23, 2021
@uzaysan
Copy link

uzaysan commented Oct 2, 2021

httpRequest has issues. Sometimes request fails or constructed in worng way and server returns 4XX response. Axios works fine. I think we should remove it.

@mtrezza
Copy link
Member

mtrezza commented Oct 2, 2021

If it is already severely broken in the way that you describe, where requests sometimes fail, we probably should make a small change to use axios, even while it's being deprecated. Because deprecation means it will still be available for a year. Not sure how broken it is though.

@uzaysan
Copy link

uzaysan commented Oct 2, 2021

If it is already severely broken in the way that you describe, where requests sometimes fail, we probably should make a small change to use axios, even while it's being deprecated. Because deprecation means it will still be available for a year. Not sure how broken it is though.

On the back4app support group on slack someone faced this problem. Syntax was correct but server wasn't returning the response it should. İ suggested him to use node-fetch or axios to see if problem is under default httpRequest and he said problem is fixed when using axios. And like you said, one can easily add their preferred http library. Maintaining a default one should be avoided in my opinion.

@mtrezza
Copy link
Member

mtrezza commented Oct 2, 2021

I see. I think we have 3 options:

  • a) Remove it immediately with Parse Server 5, which will break many deployments for which http currently works fine (I understand it only has issues with specific requests?)
  • b) Deprecate it in v5 and remove it in v6
  • c) Same as (b) and additionally replace the http library with axios while deprecating it

Which would you prefer?

@uzaysan
Copy link

uzaysan commented Oct 2, 2021

I think option b makes sense. Option c is also possible but imo there is no point of fixing a bug of a deprecated feature.

@dblythy
Copy link
Member Author

dblythy commented Oct 2, 2021

I would like to see it, and Parse.Cloud.useMasterKey() removed completely, but I understand how our community has responded to breaking changes in the past. If you are currently using it for simple requests, it’s probably working fine and having to migrate straight away could be frustrating.

I think option b is the best, we could go with c but I think having direct aliases to other packages could result in developers assuming issues and documentation related with Parse.Cloud.httpRequest are our responsibility, rather than axios’.

@mtrezza
Copy link
Member

mtrezza commented Oct 2, 2021

I think we agree on (b) then, sounds good to me.

@mtrezza
Copy link
Member

mtrezza commented Oct 9, 2021

Closing via #7595

@mtrezza mtrezza closed this as completed Oct 9, 2021
@mtrezza mtrezza reopened this Oct 19, 2021
@mtrezza
Copy link
Member

mtrezza commented Oct 19, 2021

Reopened to discuss:

@dblythy I just noticed that httpRequest is still used internally for hooks in HooksController.js, even if we do not expose it via Parse.Cloud.httpRequest. So the original issue of the deprecated legacy module is not solved by only removed public accessibility. That means (a) is not really an option, and we'd have to do (c) in some form I suppose. In any case, I still think deprecating the public method in #7595 made sense.

@dblythy dblythy mentioned this issue Oct 12, 2022
31 tasks
@mtrezza mtrezza changed the title Depreciate Parse.Cloud.httpRequest Deprecate Parse.Cloud.httpRequest Oct 12, 2022
@mtrezza mtrezza changed the title Deprecate Parse.Cloud.httpRequest Replace outdated http request dependency with axios Nov 2, 2022
@mtrezza
Copy link
Member

mtrezza commented Nov 2, 2022

I've renamed the title and objective of this issue to replace the outdated http request dependency with axios. You gave many good reasons why we should replace it. We have already deprecated it in #7595 and will remove its exposure in #8271. It seems that we still need to replace it, since it's also used internally.

@mtrezza mtrezza linked a pull request Nov 3, 2022 that will close this issue
4 tasks
@mtrezza mtrezza changed the title Replace outdated http request dependency with axios Replace outdated http request dependency with Node 18 fetch Nov 4, 2022
@mtrezza mtrezza changed the title Replace outdated http request dependency with Node 18 fetch Replace outdated http request dependency with Node 18's fetch Nov 4, 2022
@mtrezza
Copy link
Member

mtrezza commented Nov 4, 2022

Node 18 provides a native method fetch that should make it unnecessary to add a dependency for http requests. Parse Server 7 will support Node >=18 (Node 16 EOL will be Sept 2023), and from then on we can switch to native fetch.

More than 500 CI tests are currently using the request lib. Migrating to axios and possibly introducing bugs or flaky tests doesn't seem a necessary effort. Even though the request lib is outdated, the issues reported by developers were when using the Parse.Cloud.httpRequest method, which will be removed anyway. Parse Server internally and the tests seem to work fine with the request lib. So we won't touch it for now.

From Parse Server 7 (2024) onwards, we can migrate to the native fetch method. Since it will be only internal, it won't be breaking and we can migrate gradually. Until then, this issue is on hold.

@ardatan
Copy link

ardatan commented Nov 9, 2022

Maybe you can try @whatwg-node/fetch (GraphQL Yoga uses it under the hood) which ponyfills Fetch API only if it doesn't exist in the environment.
https://github.com/ardatan/whatwg-node/tree/master/packages/fetch

I'd love to help you with this if you decide to give it a try.

@Urigo
Copy link

Urigo commented Nov 9, 2022

@ardatan I'm also linking the blog post you wrote about it: https://the-guild.dev/blog/fetch-for-servers

@mtrezza
Copy link
Member

mtrezza commented Nov 9, 2022

I'm skeptical given the amount of changes we'd have to make throughout the repo and that only to bridge 2023. Unless there is another benefit?

@Urigo
Copy link

Urigo commented Nov 14, 2022

@mtrezza we are not sure it would be a big effort, are you open for us trying out and see if we can come up with a PR for it before Parse 6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants