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

Fix: remove cross-fetch #32

Merged
merged 3 commits into from
Jun 1, 2023
Merged

Fix: remove cross-fetch #32

merged 3 commits into from
Jun 1, 2023

Conversation

rolznz
Copy link
Contributor

@rolznz rolznz commented May 17, 2023

Alternative to #30

@rolznz rolznz changed the title Fix/remove cross fetch Fix: remove cross-fetch May 17, 2023
README.md Show resolved Hide resolved
@@ -1,4 +1,3 @@
import fetch from 'cross-fetch';
Copy link
Contributor

Choose a reason for hiding this comment

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

in the constructor of LightningAddress we could check if global.fetch is defined, if not throw a nice error message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

globalThis might not even exist, and then the check wouldn't work as far as I know. I think it is easier for users to figure out the issue if they get an error directly from the failed usage of fetch

Copy link
Contributor

Choose a reason for hiding this comment

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

afaik globalThis always exists, doesn't it? and I am not sure if people figure that out.
if you use a library you expect it to work, otherwise you blame the library mostly.

Is a check like this a bad practice?

if (!globalThis.fetch) { throw new Error("Fetch is missing. Please install a polyfill"); } 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should do this. We have to add this check everywhere we use fetch. In the alby-js-sdk repo we already have a section where we address this issue - I think is a better solution: https://github.com/getAlby/alby-js-sdk#for-nodejs

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.

2 participants