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 with cross-fetch #118

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Conversation

perrin4869
Copy link
Collaborator

This thing drove me crazy lol
Working on updates to the miniapp api, I noticed that requests to gpp stopped working with newer univapay-node versions.
After lots of digging, found that the cause was the upgrade to isomorphic-fetch here.
isomorphic-fetch@3 uses node-fetch@2, which introduced an esm module. When bundling with rollup, it would import node-fetch as a namespace, but isomorphic-fetch treated it as a cjs import. Long story short, everything beaks.
There is a fix pending in isomorphic-fetch matthew-andrews/isomorphic-fetch#195, but doesn't seem like it'll be merged soon. On the other hand, cross-fetch already has the fix merged in and seems better maintained...

@perrin4869
Copy link
Collaborator Author

The other option, which I think I prefer, is to leave out isomorphic-fetch entirely, and leave it to the end users to polyfill fetch if they need to. If we go with that option, we need to document in the README which polyfills will be necessary.

@phieronymus
Copy link
Collaborator

Sorry for that. Seems that I am the one who broke this up. I did not notice the two versions in the package-lock.

To be honest isomorphic-fetch has not been updated for a few years now. I believe that changing the package is a good idea.

I also believe that letting the consumer polyfill would be a good idea though. We do not have this issue on our application but some user may want to use other polyfill that work with their own sdk as well 🤔

@perrin4869
Copy link
Collaborator Author

The other thing is that many applications don't even need to polyfill - if the app is targeting modern browsers then isomorphic-fetch is being wasted...

@perrin4869
Copy link
Collaborator Author

Oh and don't worry, this was a very specific problem that wouldn't be noticed unless you're having a setup like in the services-typescript repo :P

Copy link
Contributor

@laxa88 laxa88 left a comment

Choose a reason for hiding this comment

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

LGTM

@perrin4869
Copy link
Collaborator Author

OK, let's merge this in for now, and think about what to do about polyfilling before v1

@perrin4869 perrin4869 merged commit f36ad91 into master Jan 15, 2021
@perrin4869 perrin4869 deleted the bugfix/cross-fetch branch January 15, 2021 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants