I think it looks good. While the PR description is good/clear, it would be easier to analyse the code if a separate PR was opened for each issue (appreciate that it's not always possible or makes…
Ok, thanks.
What is the reason to add break
at the end of the last loop? Isn't the loop going to finish naturally because you increment i
?
A bit of a nitpick, but I would also suggest…
You could have an entity FileServer:
url: string, read: boolean, write: boolean
servers
would hold an array of FileServer objects.
It would be better to wrap the whole body of the function in the try catch
block, rather than just catch an error in this one instance.
I think Servers
is too ambiguous to use as a type. It should either be FileServer
or BlossomServer
. FileServer
is a better option if we want to allow for a possibility that in the future there will be other compatible standards other than Blossom.
You don't need this if statement. getHash
returns a string or null, so you can directly assign hash
instead of re-assigning null
.
The same question as before - do we need noFiles
, and if yes, can you some comment as to why.
Sounds like a pretty critical issue. Can you please add more information and create an issue?
Similar to my point elsewhere in the PR, I don't think we should be storing blossoms in a map. It should be an array instead.
I don't think you need to reject the promise in this scenario. You can return a resolved promise with either true
(if you received the expected response) or false
, and you won't need to do rely on catching the error in the client code to make a business logic decision.
You should use either the async await syntax or promise chaining, not both of them at the same time.
Should it be added at the end of the array instead? It would make sense, going from the first version to the last version, i.e. 1, 2 .... n
Why not keep the existing property, blossomUrls
? It looks like the contect is exactly the same, an array of strings.