Skip to content
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

Merged
merged 11 commits into from
Dec 11, 2020

Conversation

owenpearson
Copy link
Member

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.

Copy link
Contributor

@kavalerov kavalerov left a 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.

Copy link
Contributor

@QuintinWillison QuintinWillison left a 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.

@@ -0,0 +1,20 @@
name: ably-asset-tracking-js CI
Copy link
Contributor

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 @@
{
Copy link
Contributor

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
Copy link
Contributor

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.

@kavalerov
Copy link
Contributor

@owenpearson can we merge this please?

@owenpearson owenpearson merged commit 7df6250 into main Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants