-
Notifications
You must be signed in to change notification settings - Fork 8
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
Initial implementation of AssetSubscriber class #1
Conversation
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.
Looks good to me. I suggest adding new things in a separate PR on top of this one, to make things more atomic, if that's ok.
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.
Nothing major so happy to defer some or all to a later PR if you prefer to get this landed.
.github/workflows/actions.yml
Outdated
@@ -0,0 +1,20 @@ | |||
name: ably-asset-tracking-js CI |
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.
For what it's worth, given name
is optional at the root of these files, I opted to leave it out entirely and use the file name to convey purpose (e.g. here (check) and here (assemble)). It's repetition, in my opinion.
As such, I would be tempted to rename this file to fit purpose (is this equivalent to our Android check
?).
Also, TIL, npm-ci. 😁
@@ -0,0 +1,7 @@ | |||
{ |
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.
Might it be worth also having an .editorconfig
file also?
We have one in the Android repository.
@@ -0,0 +1,32 @@ | |||
### Usage |
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.
I think perhaps a top-level (##
, H2) header to intro the repository would be nice from outset.
@owenpearson can we merge this please? |
Largely a direct clone of the Kotlin version. The public API is presented by example in README.md - the only major difference here is that the
AssetSubscriber
class uses an options pattern rather than a builder pattern, although this can easily be changed if needed.