[web] Add web Logger implementation
ClosedPublic
Actions

Authored by kamil on Jul 19 2023, 3:01 AM.

Details

Reviewers

michal
tomek
• jon

Commits

rCOMM147eeb078d0a: [web] Add web Logger implementation

Summary

To use the same Logger API we have on native rn.

Not sure what will be the best place to put this file, but I think it's better to separate and avoid adding "web" implementation in native/ directory.

Test Plan

Check if clang format works
Check if Logger works! is visible on worker console

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Jul 19 2023, 3:01 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptJul 19 2023, 3:01 AM

kamil added inline comments.Jul 19 2023, 3:03 AM

web/scripts/run_emscripten.sh
104 ↗	(On Diff #28819)	this is hiding file, comment to show

kamil added a child revision: D8549: [native] Make metadata related methods public.Jul 19 2023, 3:15 AM

Harbormaster completed remote builds in B21024: Diff 28819.Jul 19 2023, 3:24 AM

kamil published this revision for review.Jul 19 2023, 8:59 AM

• jon requested changes to this revision.Jul 19 2023, 12:10 PM

• jon added inline comments.

native/cpp/CommonCpp/DatabaseManagers/CommQueryExecutor.cpp
25 ↗	(On Diff #28819)	Could make this a bit more contextual.
web/cpp/Logger.cpp
1 ↗	(On Diff #28819)	Ideally `../../native/cpp/CommonCpp/` would be in the include path
web/scripts/run_emscripten.sh
86 ↗	(On Diff #28819)
104 ↗	(On Diff #28819)	to delete the "backup file"

This revision now requires changes to proceed.Jul 19 2023, 12:10 PM

address review

native/cpp/CommonCpp/DatabaseManagers/CommQueryExecutor.cpp
25 ↗	(On Diff #28819)	This class will be removed soon anyway, I wanted to show how I tested this, but I'll update this as you suggested
web/cpp/Logger.cpp
1 ↗	(On Diff #28819)	Good call - thanks. I will add `Logger.h` to avoid updating this at the end of the stack (because that's the way it is included in `SQLiteQueryExecutor` and `#include "Tools/Logger.h"` will cause compilation error)
web/scripts/run_emscripten.sh
104 ↗	(On Diff #28819)	if I recall correctly you requested to add this to `.gitignore` and not delete (for some reason), but okay and by "hiding file" I meant hiding this by Phabricator, it is caused because of the `@generated` phrase

michal accepted this revision.Jul 20 2023, 7:43 AM

Harbormaster completed remote builds in B21086: Diff 28891.Jul 20 2023, 7:56 AM

• jon requested changes to this revision.Jul 20 2023, 8:53 AM

• jon added inline comments.

web/scripts/run_emscripten.sh
86 ↗	(On Diff #28819)	Don't think this was addressed
104 ↗	(On Diff #28819)	Either way is fine with me.

This revision now requires changes to proceed.Jul 20 2023, 8:53 AM

kamil requested review of this revision.Jul 20 2023, 9:00 AM

kamil added inline comments.

web/scripts/run_emscripten.sh
86 ↗	(On Diff #28819)	explained in https://phab.comm.dev/D8548?id=28819#inline-55010 when I will do it, this line will cause compilation error in D8553