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.

Sign up for GitHub

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

Support observeProperty in binding-firestore #808

Merged
merged 3 commits into from
Aug 9, 2022
Merged

Support observeProperty in binding-firestore #808

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0f2449
make observeProperty support #807
hidetak Jul 17, 2022
c3ae13e
reduce logs
hidetak Jul 17, 2022
6e2a946
modified log headers and lint warnings
hidetak Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/binding-firestore/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,8 @@ The WoT operations that can be implemented for Client as follows.
| readMultipleProperties | - |
| writeProperty | ✓ |
| writeMultipleProperties | - |
| observeProperty | - |
| unobserveProperty | - |
| observeProperty | ✓ |
| unobserveProperty | ✓ |
| invokeAction | ✓ |
| emitEvent | N/A |
| subscribeEvent | ✓ |
Expand Down
2 changes: 1 addition & 1 deletion packages/binding-firestore/src/firestore-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const writeDataToFirestore = async (
content: Content,
reqId: string = null
): Promise<void> => {
console.debug("[debug] writeDataToFirestore topic:", topic, " value:", content, reqId);
console.debug("[debug] writeDataToFirestore topic:", topic, reqId);
Copy link
Member

Choose a reason for hiding this comment

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

We seemed to have missed it before but we usually add the "protocol" so that we know where the log comes from and we can filter it..

e.g., console.debug("[binding-http]", HttpServer ...);

Do you think we can do this for firestore too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielpeintner

Thank you for giving comment !

Looks good but I added a logging comment and wanted to ask you about resolving warnings like
"Unexpected any. Specify a different type".

How can I check the above message?

Do you think we can do this for firestore too?

OK. I will modify the log to output [binding-firestore].

Copy link
Member

Choose a reason for hiding this comment

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

Looks good but I added a logging comment and wanted to ask you about resolving warnings like
"Unexpected any. Specify a different type".

How can I check the above message?

Visual Studio code should show you these warnings.
Besides that, running npm run lint in the root folder or for example in packages/binding-firestore will show you these warnings as well.
Moreover, the "Files changed" tab of Github highlights the same issues

Do you think we can do this for firestore too?

OK. I will modify the log to output [binding-firestore].

👍

Copy link
Member

Choose a reason for hiding this comment

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

W.r.t. debugging.

We are about to change debug module for logging.
see #783

const ref = firestore.collection("things").doc(encodeURIComponent(topic));
const data = { updatedTime: Date.now(), reqId, content: "" };
if (content && content.body) {
Expand Down
17 changes: 6 additions & 11 deletions packages/binding-firestore/src/firestore-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ export default class FirestoreServer implements ProtocolServer {
console.debug(
`[debug] FirestoreServer at ${this.getHostName()} received message for '${topic}'`
);
console.debug(`[debug] writing property(${propertyName}) content: `, content);
console.debug(`[debug] writing property(${propertyName})`);
const options: WoT.InteractionOptions & { formIndex: number } = {
formIndex: ProtocolHelpers.findRequestMatchingFormIndex(
property.forms,
Expand Down Expand Up @@ -247,12 +247,8 @@ export default class FirestoreServer implements ProtocolServer {
}

const retContent = await thing.handleReadProperty(propertyName, options);
console.debug(`[debug] getting property(${propertyName}) data: `, retContent);
console.debug(`[debug] getting property(${propertyName})`);
await writeDataToFirestore(this.firestore, propertyReadResultTopic, retContent, reqId);
if (thing.properties[propertyName].observable) {
// TODO: Currently, observeProperty is not supported, so it will be implemented after it is supported.
// await writeDataToFirestore(this.firestore, topic, retContent, reqId);
}
}
);
}
Expand Down Expand Up @@ -298,16 +294,15 @@ export default class FirestoreServer implements ProtocolServer {
const outContent = await thing
.handleInvokeAction(actionName, content, options)
.catch((err) => {
// when data is registered in the firestore, the callback may be called multiple times,
// in which case here is called
console.error(
`[error] FirestoreServer at ${this.getHostName()} got error on invoking '${actionName}': ${
`[warn] FirestoreServer at ${this.getHostName()} got error on invoking '${actionName}': ${
err.message
}`
);
console.error(err);
});
// Firestore cannot return results
console.warn(
`[warn] FirestoreServer at ${this.getHostName()} cannot return output '${actionName}'`
);
await writeDataToFirestore(
this.firestore,
actionResultTopic,
Expand Down
10 changes: 7 additions & 3 deletions packages/binding-firestore/test/firestore-client-basic-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,11 @@ class FirestoreClientBasicTest {
@test async "[client] subscribe and unsubscribe event with object"() {
let subscribeFlg = true;
let errorMes = null;
let retVal = null;
const sub = await thing.subscribeEvent("eventObject", async (event) => {
if (subscribeFlg) {
const v = await event.value();
assert.deepEqual(v, { eventStr: "event1", eventNum: 123 });
retVal = v;
} else {
errorMes = "called but unsubscribed";
}
Expand All @@ -217,6 +218,7 @@ class FirestoreClientBasicTest {
eventNum: 123,
});
await wait(500);
assert.deepEqual(retVal, { eventStr: "event1", eventNum: 123 });
sub.stop();
subscribeFlg = false;
await thing.invokeAction("actionEventObject", {
Expand All @@ -227,20 +229,22 @@ class FirestoreClientBasicTest {
assert.equal(errorMes, null);
}

@test.skip async "[client] observe and unobserve property *observeProperty is currently unavailable"() {
@test async "[client] observe and unobserve property"() {
let observeFlg = true;
let errorMes = null;
let retVal = null;
const ob = await thing.observeProperty("stringProperty", async (str) => {
if (observeFlg) {
const v = await str.value();
assert.equal(v, "test-string-888");
retVal = v;
} else {
errorMes = "called but unobserved";
}
});
await wait(500);
await thing.writeProperty("stringProperty", "test-string-888");
await wait(500);
assert.strictEqual(retVal, "test-string-888");
ob.stop();
observeFlg = false;
await thing.writeProperty("stringProperty", "test-string-889");
Expand Down
3 changes: 3 additions & 0 deletions packages/binding-firestore/test/test-thing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,17 @@ export const launchTestThing = async (): Promise<WoT.ExposedThing | void> => {
thing.setPropertyWriteHandler("objectProperty", async (value) => {
const v = (await value.value()) as Record<string, unknown>;
objectProperty = v;
await thing.emitPropertyChange("objectProperty");
});
thing.setPropertyWriteHandler("stringProperty", async (value) => {
const v = (await value.value()) as string;
stringProperty = v;
await thing.emitPropertyChange("stringProperty");
});
thing.setPropertyWriteHandler("integerProperty", async (value) => {
const v = (await value.value()) as number;
integerProperty = v;
await thing.emitPropertyChange("integerProperty");
});

// set action handlers
Expand Down
6 changes: 3 additions & 3 deletions packages/examples/src/bindings/firestore/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
`npm install`
1. start firebase emulator
example is executed using the firebase emulator.
`$ npm start fbemu`
`$ npm run fbemu`
1. start counter-thing
Start the counter-thing example.
`$ npm start example:counter-thing`
`$ npm run example:counter-thing`
1. start counter-client
Start counter-client and access counter-thing.
`$ npm start example:counter-client`
`$ npm run example:counter-client`

If you would like to run the program in Firebase instead of in the Firebase emulator, please do the following.

Expand Down
5 changes: 5 additions & 0 deletions packages/examples/src/bindings/firestore/counter-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ const main = async () => {
let c = await event.value();
console.info("count from event is ", c);
});
// observe property
const obProperty = await thing.observeProperty("count", async (count) => {
let c = await count.value();
console.info("count from observe property is ", c);
});
// read property #1
const read1 = await thing.readProperty("count");
console.log("count value is", await read1.value());
Expand Down
3 changes: 3 additions & 0 deletions packages/examples/src/bindings/firestore/counter-thing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ const main = async () => {
count = newValue;
lastChange = new Date().toISOString();
thing.emitEvent("change", count);
thing.emitPropertyChange("count");
return undefined;
});
thing.setActionHandler("decrement", async (params, options) => {
Expand All @@ -169,13 +170,15 @@ const main = async () => {
count = newValue;
lastChange = new Date().toISOString();
thing.emitEvent("change", count);
thing.emitPropertyChange("count");
return undefined;
});
thing.setActionHandler("reset", async (params, options) => {
console.log("Resetting count");
count = 0;
lastChange = new Date().toISOString();
thing.emitEvent("change", count);
thing.emitPropertyChange("count");
return undefined;
});
// expose the thing
Expand Down