-
Notifications
You must be signed in to change notification settings - Fork 28
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
refactor!: ky replaces axios #390
Conversation
import { wrapBytesWithHelpers } from '../utils/bytes' | ||
|
||
const endpoint = '/bytes' | ||
const endpoint = 'bytes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was very convenient that I could search for certain endpoints both in the Bee codebase and in bee-js by having the slash and immediately find the right place where it is used. Would it be possible to keep this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I understand your strategy and it makes sense, unfortunately, I don't think it is possible. I would otherwise kept it like that. See https://github.com/sindresorhus/ky#prefixurl:
Leading slashes in input are disallowed when using this option to enforce consistency and avoid confusion about how the input URL is handled, given that input will not follow the normal URL resolution rules when prefixUrl is being used, which changes the meaning of a leading slash.
Alternative to your strategy now can be using '
/ = '
instead of /
;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Two more things to my review comments:
I've already turned off @typescript-eslint/no-non-null-assertion
eslint rule in mantaray-js
, maybe I would do the same here. From the code it is obvious the properties are defined so you can do non-null assertions.
Other thing is the ky
instance is required on every exported module seems like an inconvenient downgrade for me.
It could accept string
type as well as so far with httpBase: string | Ky
as a first parameter instead of it only accepts ky: Ky
. Therefore, this breaking change wouldn't be that breaking (as you had to change every module tests because of this).
Moreover, you said when we exported the AxiosOptions
we shouldn't stuck to one HTTP library implementation and our library should abstract it as much as possible. Nevertheless this principal is not applied in this case.
Co-authored-by: nugaon <50576770+nugaon@users.noreply.github.com>
Co-authored-by: nugaon <50576770+nugaon@users.noreply.github.com>
Thanks, @nugaon for the review and a lot of great improvement suggestions!
Honestly, I don't agree with this. First of all the modules are exported, but they are internal to the package so it is up to us how we want to implement this. Second of all, Ky instance does define more than the URL that it talks to. It defines hooks, default headers, and in future things like retries, timeout and other configurations. If we allow passing a string, we open a space for a future error where a developer can pass URL string instead of Ky which removes all the configuration. Allowing string seems to me like a "hack" to avoid doing more work (eq. refactoring tests) that I have already done anyway(and which was not hard at all as most of refactoring did my IDE). I would prefer to have clean solution.
I don't see the parallel here. I said that regarding exposed AxiosOptions on the public API. Modules are not public API so I don't see how it is related here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After our discussion we agreed on we won't support usage of modules directly and those can be seen as private inside modules. Thereby interface changes on the modules won't cause breaking changes and we won't expose them in the future.
If we take the bargain - and we should do that IMO - to have streaming possibilities with fetch
based HTTP library instead of having axios
with uploadProgress functionality (on client-side), then let's roll it out.
little refactor proposal: within the modules at the API path construction the base path constant of the endpoint called many places called *endpoint
. we could name it like basePath
instead of that.
Closes #373
As this work will go to 2.0 version, I have created a
2.0
branch that this PR targets that will be used for future works until Bee is released and1.2
version will be possible to release. Or if I gonna finish all the work for2.0
before1.2
release then I guess will skip it. Will see.Breaking changes:
bee-js
are now reported withUser-Agent: bee-js/<<bee-js's version>>
Utils.setDefaultHeaders()
was removed in favor ofBee
/BeeDebug
instance's optiondefaultHeaders
Utils.hooks.*
was removed in favor ofBee
/BeeDebug
instance's optionsonRequest
andonResponse
WHATWG ReadableStream
. If you need NodeJS's Readable you can useUtils.Stream.readableWebToNode()
utility function.axiosOptions
were removed from those methods that supported it (for examplebee.downloadReadableData()
,bee.reuploadPinnedData()
,UploadOptions
does not haveaxiosOptions
property anymore)fetch
does not support tracking of upload progress (like XHR/axios supported with theonUploadProgress
).Other changes: