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

Support observeProperty in binding-firestore #808

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

hidetak
Copy link
Contributor

@hidetak hidetak commented Jul 17, 2022

I was able to deal with it only by modifying the test, example code and README.

@hidetak hidetak changed the title Support observeProperty in binding-firestore #807 Support observeProperty in binding-firestore Jul 17, 2022
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

Thanks for your update!

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

We try to keep the code warning free. If really needed you can also an ignore warning.
Thanks!

@@ -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

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM.

We can update the logging in a follow-up PR.
Note: we are bout to change logging to use the debug library.

@relu91 any concerns before merging?

@danielpeintner
Copy link
Member

checked with @relu91. Good to go.

@danielpeintner danielpeintner merged commit 5815055 into eclipse-thingweb:master Aug 9, 2022
@hidetak
Copy link
Contributor Author

hidetak commented Aug 9, 2022

@danielpeintner
Thank you for checking and merging !

@danielpeintner
Copy link
Member

@hidetak thank you for your contribution.

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