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 isomorphic-fetch #1908

Merged
merged 17 commits into from
Mar 26, 2025
Merged

Replace isomorphic-fetch #1908

merged 17 commits into from
Mar 26, 2025

Conversation

boray
Copy link
Member

@boray boray commented Nov 19, 2024

Fixes #1904

This PR replaces outdated isomorphic fetch with built-in fetch and updates the fetch examples to devnet from berkeley.

@mitschabaude
Copy link
Contributor

Why do we need this instead of using built-in fetch?

@ymekuria
Copy link
Contributor

Why do we need this instead of using built-in fetch?

I agree with @mitschabaude here. We should use the built-in fetch instead of adding a dependancy. We moved to node fetch in the zkApp-CLI as well.

@boray
Copy link
Member Author

boray commented Nov 20, 2024

Why do we need this instead of using built-in fetch?

The main reason is to not drop support for node versions <18.

@mitschabaude
Copy link
Contributor

Why do we need this instead of using built-in fetch?

The main reason is to not drop support for node versions <18.

I think we can do us a favor and drop that support!

@boray boray marked this pull request as ready for review November 20, 2024 13:56
@boray boray requested review from a team as code owners November 20, 2024 13:56
@boray boray requested review from 45930 and querolita November 20, 2024 13:56
Copy link
Contributor

@45930 45930 left a comment

Choose a reason for hiding this comment

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

I think we should include a changelog entry for this, but otherwise LGTM!

@boray
Copy link
Member Author

boray commented Nov 20, 2024

I think we should include a changelog entry for this, but otherwise LGTM!

You're right, we're dropping support for node versions <18 with the last commit, so this needs to be included in the changelog. Also, this breaks backward compatibility. Should we wait for v3 to release this PR?

@boray boray added the breaking label Nov 21, 2024
@mitschabaude
Copy link
Contributor

I'd consider it not breaking, and would release it under a patch version, because we have been explicit about not supporting node < 18 before:

@45930
Copy link
Contributor

45930 commented Nov 27, 2024

I suppose we need to be more explicit about our definition of breaking. I'm inclined to agree with Gregor that we are very explicit already about node 18+ being required. As we release major versions in the future, we may want to aggressively bump that supported node version so we can continue to make changes like these.

I'm ok to merge this in a minor version. Even though I see the argument that we're "dropping support", I don't think that we actually had previously "supported" those versions. They just happened to work.

@Trivo25 Trivo25 removed the breaking label Feb 23, 2025
@boray boray requested a review from 45930 March 18, 2025 18:45
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

please update the npm-deps-hash

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@boray boray dismissed github-actions[bot]’s stale review March 19, 2025 00:28

This bot is not checking the npmDepsHash but only request changes if it's wrong. It's not possible to make the bot approve since the related workflow is only triggered when npmDepsHash is not correct.

@boray boray merged commit 616bec5 into main Mar 26, 2025
30 checks passed
@boray boray deleted the chore/replace-isomorphic-fetch branch March 26, 2025 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove isomorphic-fetch dependency
5 participants