-
Notifications
You must be signed in to change notification settings - Fork 311
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
chore(deps): remove cross-fetch
polyfill
#597
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this is perfect, been waiting a long time for this day, and I agree it's time now. 🙌
@jasonkuhrt Also TY for the fix 👏🏻 @jonkoops |
@LuizAsFight I'll take 1% for the management aspect but 99% to @jonkoops for this :D Regarding release, since this is a significant breaking change, once the signal on the pre-release is good, we can cut a stable. Any help with that appreciated :) |
@jasonkuhrt if you are thinking of cutting a major release, are there any other breaking things you would like to include? If so I can volunteer some time to help out with any of those. I can see from the README that CommonJS might be a potential one, but I don't know if that is something you are eyeing for this cycle. |
@jonkoops nice thanks, I have lots of big picture things but not sure what smaller aspects could into the next major release. I'll try to refresh on this some time today and drop feedback about it here. |
Hey, so this #509 contains a breaking change. But it's not exactly a small feature. I didn't see anything else after a quick glance though I would be surprised if there's nothing out there in the issue tracker, I just don't have time to comb through it. |
Perhaps we could add a 'breaking' label that can be assigned to issues/PRs. If you want you can give me permission to manage issues and I can go through the backlog to make sure all relevant issues are labeled. Then once that is done we can triage them and determine if any should land this cycle. WDYT? |
@jonkoops I haven't done a triage of the issues in a while so I would appreciate that. I don't have too much time right now so if we can focus on breaking changes that fix bugs that would be I think the right approach for this. In other words, generally avoiding issues that would involve library design and API thinking. Invite sent! |
@jasonkuhrt Let's continue the discussion under #602, I don't want to clutter this PR with more chatter. |
Just a question, when is the plan to release an update to the package with this PR included. |
@martinrojas No plan per-se. There are pre-releases if you're needing them right now. |
I am trying to go through the open issues and make sure we have nothing missing for the next release, as well as get as many open issues resolved as reasonably possible. It might take a little, as I am not doing this on my employers time. Help is always appreciated, so if you see some issues that can be resolved with a PR, feel free to submit one. |
**Problem** At some point, the sdk-core/subgraph test cases started to fail with "socket hang up" error when querying subgraph. After some research, it was discovered that it is due to the usage of an older version of the "cross-fetch" polyfill which didn't support node 20+, see [here](graffle-js/graffle#628) and [here](graffle-js/graffle#597). **Solution** The newer cross-fetch project solved this. The solution is to fix the "resolution" of the cross-fetch in this project to the latest, for now. Additionally, this PR also: a) include some minor refactoring around the test setup process, b) test against both node 18 and 20 for SDK core, and test against node 20 for subgraph.
The Fetch API has been marked as stable in Node.js 21, and is supported experimentally without the need to opt-in in under all actively maintained LTS versions (v18, v20). This means that the
cross-fetch
polyfill is no longer required in order to support Node.js.Closes #326
Closes #216
Closes #545
Closes #448
Closes #402
Closes #99
Closes #41
This PR might also impact #332, #31