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

chore(deps): remove cross-fetch polyfill #597

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

jonkoops
Copy link
Collaborator

@jonkoops jonkoops commented Oct 27, 2023

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

Copy link
Member

@jasonkuhrt jasonkuhrt left a 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 jasonkuhrt merged commit 8566e85 into graffle-js:main Oct 27, 2023
1 of 7 checks passed
@LuizAsFight
Copy link

LuizAsFight commented Oct 27, 2023

@jasonkuhrt
when can we expect a new release? would be nice to remove workarounds around versioning to avoid this issue 😀

Also TY for the fix 👏🏻 @jonkoops

@jonkoops jonkoops deleted the drop-cross-fetch branch October 27, 2023 14:32
@jasonkuhrt
Copy link
Member

@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 :)

@jonkoops
Copy link
Collaborator Author

jonkoops commented Oct 27, 2023

@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.

@jasonkuhrt
Copy link
Member

@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.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 28, 2023

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.

@jonkoops
Copy link
Collaborator Author

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?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Oct 29, 2023

@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!

@jonkoops
Copy link
Collaborator Author

@jasonkuhrt Let's continue the discussion under #602, I don't want to clutter this PR with more chatter.

@martinrojas
Copy link

Just a question, when is the plan to release an update to the package with this PR included.

@jasonkuhrt
Copy link
Member

@martinrojas No plan per-se. There are pre-releases if you're needing them right now.

@jonkoops
Copy link
Collaborator Author

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.

@jonkoops jonkoops mentioned this pull request Dec 18, 2023
alexkim205 pushed a commit to alexkim205/graphql-request that referenced this pull request Feb 9, 2024
hellwolf added a commit to superfluid-finance/protocol-monorepo that referenced this pull request Aug 29, 2024
**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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment