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

Client API update #125

Merged
merged 71 commits into from
Dec 12, 2019
Merged

Client API update #125

merged 71 commits into from
Dec 12, 2019

Conversation

dmitrykuzmin
Copy link
Contributor

@dmitrykuzmin dmitrykuzmin commented Dec 10, 2019

This PR updates the API of Spine web client.

Now, the API is a bit more flexible and tries to mimic the new Java API where appropriate.

In general, the new API looks as follows:

// Query.
client.select(Customer.class)
      .byId(westCoastCustomerIds())
      .withMask("name", "address", "email")
      .where([Filters.eq("type", "permanent"),
              Filters.eq("discount_percent", 10),
              Filters.eq("company_size", Company.Size.SMALL)])
      .orderBy("name", OrderBy.Direction.ASCENDING)
      .limit(20)
      .run(); // The returned type is `Promise<Customer[]>`.

// Subscription to entities.
client.subscribeTo(Task.class)
      .where(Filters.eq("status", Task.Status.ACTIVE))
      // Additional filtering can be done here.
      .post()
      .then(({itemAdded, itemChanged, itemRemoved, unsubscribe}) => {
          itemAdded.subscribe(_addDisplayedTask);
          itemChanged.subscribe(_changeDisplayedTask);
          itemRemoved.subscribe(_removeDisplayedTask);
      });

// Subscription to events.
client.subscribeToEvent(TaskCreated.class)
      .where([Filters.eq("task_priority", Task.Priority.HIGH),
              Filters.eq("context.past_message.actor_context.actor", userId)])
      .post()
      .then(({eventEmitted, unsubscribe}) => {
          eventEmitted.subscribe(_logEvent);
      });

// Command.
client.command(logInUser)
      .onOk(_logOk)
      .onError(_logError)
      .observe(UserLoggedIn.class, ({subscribe, unsubscribe}) => {
          subscribe(event => _logAndUnsubscribe(event, unsubscribe));
          setTimeout(unsubscribe, EVENT_WAIT_TIMEOUT);
      })
      .observe(UserAlreadyLoggedIn.class, (({subscribe, unsubscribe}) => {
          subscribe(event => _warnAboutAndUnsubscribe(event, unsubscribe));
          setTimeout(unsubscribe, EVENT_WAIT_TIMEOUT);
      })
      .post();

Notable changes

  • The client now allows to observe the events that are the immediate result of handling the posted command. This can be done using the observe method, in a similar way to the example above. Note, that in the example we make sure the event subscriptions are always cancelled, even if the event of the corresponding type is never received. This can be done in some other way but all the subscriptions created with observe should be cleaned up manually.

  • The event subscriptions will now always write spine.core.Event instances to the Firebase database and always receive spine.core.Event instances instead of event messages on the client, in the subscription objects.

  • A service which keeps the active subscriptions "alive", will no longer ignore the erroneous response received from the server. If the subscriptions can't be "kept-up", it will be removed from the list of active subscriptions and the "keep-up" requests will no longer be sent for it.

  • The entity subscription will now be notified about the no-longer-matching objects with the help of itemRemoved callback. Previously, it was done with the itemChanged callback with null body, which raised some problems when it was necessary to identify the object which stopped matching the subscription.

Changes to the old API

Removed

  • Client.fetch(...) - please use Client.select(...).byId(...).run() instead;
  • Client.subscribe(...) - please use Client.subscribeTo[Event](...).byId(...).post() instead.

Renamed

  • Client.subscribeTo(Topic) -> Client.subscribe(Topic).

Deprecated

  • Client.execute(Query) - please use Client.read(Query) instead;
  • Client.sendCommand(...) - please use Client.command(...).onOk(...) ... .post() instead.

All other API remains intact.

Other

Spine version advances to 1.2.9.

Event subscription now returns an `EventSubscriptionObject`.
The `EventSubscriptionObject` does not have redundant `itemChanged`
and `itemRemoved` callbacks and instead holds a single observable
which is named `eventEmitted` and is of `Observable<Event>` type.

The `SubscriptionBridge` now writes `Event` instances to the Firebase
database instead of event messages to allow this behavior.
The integration tests are now re-enabled, as the corresponding issue with Gretty was resolved in the latest version.
Previously, we added the node with the string value of `"null"` to the database on empty subscription update. This led to the issue when the `itemChanged` instead of `itemRemoved` callback was triggered on the client side when the entity state became no longer matching the subscription criteria.

Now, the real `null` is stored to the database on empty subscription update, which in terms of Firebase means the child node is deleted. Thus, the "correct" `itemRemoved` callback is triggered.

Also, a small bug with trying to store the `null` values with the `ImmutableMap` collector is fixed.
@dmitrykuzmin
Copy link
Contributor Author

@dmdashenkov PTAL.

The coverage is low because the changes are mostly covered by the integration tests which do not count towards coverage.

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

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

@dmitrykuzmin, as discussed, let's

  • keep the old API operational at least for some time;
  • move subscription callbacks to observe.

/**
* @protected
*/
constructor(client, requestFactory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the indent.


describe('FirebaseClient command sending', function () {

// Big timeout allows to receive model state changes during tests.
this.timeout(5000);
this.timeout(15000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be reverted? Is there a performance issue?

assert.ok(error instanceof CommandValidationError);
assert.ok(error.validationError());
// TODO:2019-06-05:yegor.udovchenko: Find the reason of failing assertion
// assert.ok(error.assuresCommandNeglected());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if this still fails.

@dmitrykuzmin
Copy link
Contributor Author

@dmdashenkov PTAL again.

*
* @abstract
*/
class ClientRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you split this file up? It's LoC count is at 600 already. GitHub even considers it too large to display.

client-js/main/client/client.js Outdated Show resolved Hide resolved
newTopic() {
return this._subscribing.newTopic();
}
constructor(querying, subscribing, commanding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, please split up this file into 2 or 3. I'd say that one class per file would be an adequate strategy.

@dmitrykuzmin
Copy link
Contributor Author

@dmdashenkov PTAL again.

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

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

@dmitrykuzmin, LGTM with a single minor comment.

@dmitrykuzmin dmitrykuzmin merged commit 96b20f5 into master Dec 12, 2019
@dmitrykuzmin dmitrykuzmin deleted the client-js-api-update branch December 12, 2019 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants