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 Annoying things with this Package. #131

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

Conversation

bertybot
Copy link

Why this Change?

This package is currently misconfigured. Running it through publint it has a few errors that need to be addressed.

The main problem being that the ESM configuration is currently misconfigured causing this package to fail to build in ESM centric environments like Vite.

  • node-fetch import is also always a require which causes ESM builds to fail.

There are also a few other annoying things with this package that should be addressed.

  • React is a peer dependency even though this package compiles to pure JS and works perfectly fine without React.
  • There is no option to supply your own fetch function basically forcing you to use 2 outdated fetch functions even if you have no need for them.

What was done

  • Correctly configured package.json
    • added exports field
    • removed deprecated module field
    • added packageManager field so people can more easily contribute to the project
  • Added optional fetchFunction property to config so that you can supply your own fetch function if you don't like the 2 supplied
  • Fixed node-fetch import
  • updated node-fetch

Conclusion

These changes most likely constitute a breaking change to users on older versions of Node. However, currently these problems currently make this package completely unusable to people on newer versions or using ESM so I think that the juice is worth the squeeze.

@bertybot bertybot requested a review from a team as a code owner August 10, 2023 19:39
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @bertybot2 to sign the Salesforce Inc. Contributor License Agreement.

@wjhsf
Copy link
Contributor

wjhsf commented Sep 29, 2023

Hi @bertybot, thanks for creating this PR! There are a lot of good suggestions in here, and we'd like to include most of them. However, now that node 18 has native fetch support, we will be able to rely on native fetch in all environments, and not need any logic for providing a custom implementation.

@echessman echessman added the ack Acknowledged label Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Acknowledged cla:missing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants