-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: add types, gh actions and remove unused methods #89
Conversation
these were copied to ipfs-core-utils
not needed anymore
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.
Did a fast pass through this. Overall, it looks good from the types perspective.
Ideally, this should be well tested against js-ipfs
, to guarantee that the HTTP/Fetch changes do not create regressions.
|
||
// validate resource type | ||
if (typeof resource !== 'string' && !(resource instanceof URL || resource instanceof Request)) { | ||
throw new TypeError('`resource` must be a string, URL, or Request') | ||
} | ||
|
||
// validate resource format and normalize with prefixUrl |
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.
Is this not needed anymore?
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.
URL below this line does the job
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.
Thanks @hugomrdias for your hard work, this is a lot. I have suggested some improvements that I think would be worth incorporating & also nit picked a bit, but feel free to ignore those.
path: string; | ||
content?: AsyncIterable<Uint8Array>; | ||
}>} | ||
*/ |
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.
This reminds me, we really should make this return a Promise<{path: string, content:AsyncIterable<Uint8Array>}>
instead (see #53)
} | ||
|
||
if (opts.json !== undefined) { | ||
if (json) { |
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.
null
is valid JSON, which would have worked without this change.
...options, | ||
method: 'POST' | ||
}) | ||
return this.fetch(resource, { ...options, method: 'POST' }) |
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.
Nit: I think line breaks are good, as they cause less noise when adding or removing an option.
@@ -166,71 +166,56 @@ class HTTP { | |||
} | |||
|
|||
/** | |||
* @param {string | URL | Request} resource | |||
* @param {APIOptions} options | |||
* @param {string | Request} resource |
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.
Nit: I think supporting URL
instances is a good idea, as unlike strings they do represent valid urls.
@@ -0,0 +1 @@ | |||
export { URL, URLSearchParams } |
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 thought TS did not liked exports of things that aren't defined locally.
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.
our config has the browser/dom stuff
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.
LGTM
BREAKING CHANGE: removed globalThis and normalise-input, format-mode and format-mtime