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

Enhance the implementation and usage of fetch API #324

Closed
baywet opened this issue Sep 13, 2020 · 10 comments
Closed

Enhance the implementation and usage of fetch API #324

baywet opened this issue Sep 13, 2020 · 10 comments
Assignees
Labels
ADO to GitHub automation label Issue caused by core project dependency modules or library enhancement

Comments

@baywet
Copy link
Member

baywet commented Sep 13, 2020

Bug Report
Prerequisites

Can you reproduce the problem?
Are you running the latest version?
Are you reporting to the correct repository?
Did you perform a cursory search?

For more information, see the CONTRIBUTING guide.
Description
isomorphic-fetch hasn't been updated in 5+ years and is not maintained anymore.
A few days ago I received a security alert on the node webhook sample indicating that node-fetch < 2.6.1 (on which isomorphic-fetch depends) had a security issue.
We should remove/replace any usage of this lib in our codebase as well as update guidance to avoid leading people into referencing a deprecated and vulnerable lib in their projects.
AB#5995

@ghost ghost added the ToTriage label Sep 13, 2020
@ddyett ddyett added enhancement ADO to GitHub automation label Issue caused by core project dependency modules or library labels Sep 14, 2020
@ghost ghost removed the ToTriage label Sep 14, 2020
@askdesigners
Copy link

Not to mention that it's a nasty monkey patch...

@nikithauc nikithauc added this to the 3.0.0 milestone Jan 31, 2021
@nikithauc
Copy link
Contributor

Any fetch polyfill can be used with the Microsoft Graph Client JS SDK and it is not specific to isomorphic-fetch.

@askdesigners what do you suggest as an alternative to the monkey patching?

@baywet
Copy link
Member Author

baywet commented Apr 7, 2021

@nikithauc since my original post isomorphic fetch has been updated to patch the security issue. However, this dependency is still pretty much inactive and we should consider an alternative.

Additionally, I can see that you have updated it for v3, but I think the samples were missed.

@matt-tyler
Copy link

I feel like it would be more ergonomic to add an optional field to the configuration object that meets the interface of the fetch API, personally.

@nikithauc
Copy link
Contributor

@matt-tyler
There is a way to set the fetch options during the client instantiation

Can you elaborate on the capabilities that you feel should be present to access the fetch API?

@alexrecuenco
Copy link

alexrecuenco commented Apr 13, 2021

Good afternoon,

I am with @matt-tyler. I may be misunderstanding the issue, but, I believe

  1. This whole module currently does not compile without the "dom" library, which should be avoided, you can import the fetch types from somewhere else alone without importing all the declarations from the dom. see ( Using /// <reference lib="dom" /> in a .d.ts typescript declaration file has global effects microsoft/TypeScript#33901 )
  2. Fetch as a global object as a requirement to run a library seems prone to errors.
  3. For backward compatibility It would not be hard to add in the library a 'fetchObject' option, and if that isn't set, you try the global fetch, and if that doesn't exist, throw an error, instead of failing midway.

I have run into issues since our linters are not catching the non-existing global objects on a NodeJs application, and some Storage one had sneaked in for months. Trying to remove the sneaking DOM library references, I have now found that THIS library doesn't let me compile typescript without them, since it is referencing the dom library

Thank you for the great work, if you need help doing this, let me know.

@nikithauc
Copy link
Contributor

@alexrecuenco Thanks a lot for this feedback! I appreciate this input as it helps in improving our library!

I will get in touch with you again during planning for the design changes of this task and would like to learn more from you.

@alexrecuenco
Copy link

@nikithauc Thank you for all the effort. Excellent tools!

@nikithauc nikithauc changed the title isomorphic-fetch should be replaced by an alternative (cross-fetch?) Enhance the implementation and usage of fetch API Apr 20, 2021
@nikithauc nikithauc removed this from the 3.0.0 milestone Apr 22, 2021
@jgburet
Copy link

jgburet commented Apr 30, 2021

Since this won't make its way to 3.0.0, could we have an example of how to use fetchOptions?
I tried to use axios (using https://github.com/lifeomic/axios-fetch) instead but I can't get that (or anything else) to work.

@nikithauc
Copy link
Contributor

Closing this issue as I opened #493 to clearly state the requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADO to GitHub automation label Issue caused by core project dependency modules or library enhancement
Projects
None yet
Development

No branches or pull requests

7 participants