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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Replace isomorphic-fetch #1908

wants to merge 4 commits into from

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
Collaborator

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
Collaborator

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
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 Issues that will lead to breaking changes label Nov 21, 2024
@mitschabaude
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Issues that will lead to breaking changes no changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove isomorphic-fetch dependency
4 participants