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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using TypeScript? #35

Open
jcbhmr opened this issue Mar 30, 2023 · 2 comments
Open

Consider using TypeScript? #35

jcbhmr opened this issue Mar 30, 2023 · 2 comments

Comments

@jcbhmr
Copy link

jcbhmr commented Mar 30, 2023

Hello! 馃憢

I've used this library in some of my Web Worker projects to download things synchronously, particularly for WASM magic stuff (sync httpGet() is nice!). I'd be interested in contributing code to this library, but I wanted to check-in with @larsgw, the author, before I do anything so I don't end up with a dead-end fork that only I use 馃槣

Specifically, here's some things that I'd love 鉂わ笍 to contribute:

  • TypeScript! 馃檶 And use multiple files.
  • Use web-focused TextEncoder or similar instead of buffer (looks like someone else is already on this in Replace 'buffer' dependency with native TextEncoder聽#27! nice!)
  • Tooling and stuff for a modern bundler like Vite or tsup
  • An improved readme with: more example code, example use case (web worker), more usage details, etc.
  • Refactor tests into multiple files.
  • Use ESM modules (possibly still compiled to CommonJS)

If you'd like to keep it steady and stable as-is, that's OK too! 馃槉 I'm just reaching out to strike up a discussion about whether or not my PRs would be stuck in limbo 馃檮

@larsgw
Copy link
Owner

larsgw commented Apr 4, 2023

Hello! Thank you very much for your interest! An improved readme is very welcome, though I do want to keep the "Limitations" section in focus because the examples will be very similar to regular fetch examples, and I would like to highlight the differences instead. Apart from that, I am inclined to avoid making things more complicated than necessary, and I am not immediately convinced right now of the necessity of TypeScript or a new bundler, especially since the bundle is just for the browser (the Node.js version needs worker.js, and by extension shared.js, to be separate files from index.js).

Use web-focused TextEncoder or similar instead of buffer (looks like someone else is already on this in #27! nice!)

As per my comment there, I do still want to keep the possibility of supporting IE and other older browsers, so I would need to make two builds. That said, browser support looks pretty good. The polyfill mentioned in the pull request seems to be a lot larger than buffer so that does not seem like a great option.

Use ESM modules (possibly still compiled to CommonJS)

As far as I understand ESM modules are by nature asynchronous which would not work here, and there are few enough imports that I think it does not really change much anyway.

Refactor tests into multiple files.

Right now the tests are copied directly from the node-fetch test (with some changes to test cases for features that are async by nature), and I would like to keep them as similar as possible. However, the tests for node-fetch 3 do seem to be separated into multiple files, so when I upgrade that could become a possibility.

I hope that all makes sense. Again, thank you for your interest.

@jimmywarting
Copy link

I think switching to esm would be good. Importing this library would not be anymore differently than loading it via a script tag. Only change needed is adding type=module
Sync fetch would still be Sync.

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

No branches or pull requests

3 participants