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

Added bun functuality #4

Merged
merged 18 commits into from
Aug 30, 2024
Merged

Added bun functuality #4

merged 18 commits into from
Aug 30, 2024

Conversation

foxyyyyyyyyyyyyy
Copy link
Contributor

I added bun functuality. You can (or you should be able to) use the package normaly. And if you use bun you need to set a /bun/src (I will further look into it to remove the src) to use the bun optimized version.

@CalumW1 CalumW1 self-requested a review August 26, 2024 00:22
@foxyyyyyyyyyyyyy
Copy link
Contributor Author

I fixed a issue in the bun version and also directly adressed the issue stated in pr #5

Copy link
Owner

@CalumW1 CalumW1 left a comment

Choose a reason for hiding this comment

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

I was able to get this working with my test application, but i had to make a couple of small adjustments to the package.json file for it run properly. Please see comments below, there is also a couple of small clean up things that i think needs to be done before the merge.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@foxyyyyyyyyyyyyy
Copy link
Contributor Author

I will Look into it

@foxyyyyyyyyyyyyy
Copy link
Contributor Author

The dist/bun probably can be removed, cause bun does not need to get compiled. In your package do you publish just the dist folder as package or everything?

@foxyyyyyyyyyyyyy
Copy link
Contributor Author

I have found a better wider compatible option. It now uses Axios instead of node fetch wich should make it compatible with a wider range of runntimes

@foxyyyyyyyyyyyyy
Copy link
Contributor Author

Also it does not require a bigger package size and should work as normal

Copy link
Owner

@CalumW1 CalumW1 left a comment

Choose a reason for hiding this comment

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

Changes look good, Axios has replaced node-fetch. I pushed a commit to change the type of the headers from AxiosRequestHeaders to RawAxiosRequestHeaders as some of the other API calls have other fields included in their headers and it was complaining about them.

@CalumW1 CalumW1 merged commit bb49e8b into CalumW1:main Aug 30, 2024
1 check passed
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