Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

refactor: convert block API to async await #1153

Merged
merged 5 commits into from
Nov 12, 2019
Merged

Conversation

alanshaw
Copy link
Contributor

@alanshaw alanshaw commented Nov 12, 2019

src/block/put.js Outdated Show resolved Hide resolved
Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

LGTM

notes:
still bugs me a bit all the

const searchParams = new URLSearchParams()
const searchParams = new URLSearchParams(options.searchParams)
if (options.format !== undefined) {
  searchParams.set('format', options.format)
}

if(options.format != null)

we have a couple of inconsistencies in there, maybe we could spend a little time finding a way to just pass options to ky instead of new URLSearchParams plus if clauses. @achingbrain you did a couple of changes to searchParams handling in ky upstream do you think we can do this already or the undefined/null is still an issue for us ?

depends on:

* [ ] #1150

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw
Copy link
Contributor Author

const searchParams = new URLSearchParams()

Agree, that shouldn't happen, we should allow them to be overridden with options on every API call. Along with timeout, signal etc.

const searchParams = new URLSearchParams(options.searchParams)

URLSearchParams allows you to pass a URLSearchParams instance OR an object as the parameter. This allows users to pass their own query string params as an object or an instance of URLSearchParams AND also allows us to add mandatory/overridden parameters from args or other options.

if (options.format !== undefined) {
  searchParams.set('format', options.format)
}

if(options.format != null)

Sometimes we check for null/undefined because a falsy check on booleans or numbers that might be 0 is not good enough. It's a matter of preference if we only consider undefined as a "value" that doesn't get added to the query string. I've been using the != null because it includes null as well as undefined and 99.9% of the time you also would not want ?arg=null which is what happens when you pass null to URLSearchParams and stringify it.

@alanshaw
Copy link
Contributor Author

@hugomrdias apologies I'm going to expand this PR to include the DAG API since it uses the block API.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@achingbrain
Copy link
Collaborator

I think we should apply the robustness principal - that is, all the if statements around URLSearchParams are verbose but it's good to only send what we know the server will support.

@alanshaw alanshaw merged commit 72fdc8c into master Nov 12, 2019
@alanshaw alanshaw deleted the refactor/block-async-await branch November 12, 2019 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants