Conversation
There's no logical reason to have the implementation dependency on the App Engine API unless we're extending the App Engine-specific APIs.
…ver the net. `instanceof` for Protos may have glitches if messages were created with different versions of Protobuf or even in different OSs. The bullet-proof method for JS is to compare TypeUrl value.
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
============================================
+ Coverage 61.88% 61.98% +0.09%
Complexity 178 178
============================================
Files 88 88
Lines 2188 2191 +3
Branches 40 40
============================================
+ Hits 1354 1358 +4
+ Misses 825 824 -1
Partials 9 9 |
|
@dmdashenkov @dmitrykuzmin PTAL. |
dmdashenkov
left a comment
dmdashenkov
left a comment
There was a problem hiding this comment.
@yuri-sergiichuk, please see my comment. Also, was there no use of AppEngine SDK at all? If so, please mention this in the PR description.
| static _newId() { | ||
| const result = new QueryId(); | ||
| result.setValue(`q-${uuid.v4()}`); | ||
| result.setValue(`q-${uuidv4()}`); |
There was a problem hiding this comment.
Is this some kind of a convention? Why do we alias this particular import?
There was a problem hiding this comment.
The default exported item is v4 and I don't think it worth using it as is. That's why I used the alias.
With just q-${v4()} it is more awkward.
| @@ -37,7 +35,7 @@ import TestEnvironment from "../../given/test-environment"; | |||
| * @return {FirebaseClient} the Firebase client instance | |||
There was a problem hiding this comment.
Are we okay with using doc-only imports in JS? My IDEA gives a warning for an unused import here, but maybe the config is wrong.
There was a problem hiding this comment.
I personally totally am. Mine shows warnings for unresolved types when the type is not imported. Also, JS through webpack and Babel has tree-shaking that'll eliminate all parts that are not being used in the runtime.
| */ | ||
| export function initClient(endpointUrl, tenantProvider, keepUpInterval) { | ||
| return spineWeb.init({ | ||
| return initSpineWeb({ |
There was a problem hiding this comment.
Maybe initSpineWebClient?
3 participants
The goal of this PR is to resolve #153 as well as upgrade our dependencies for the UI libraries. The removed AppEngine SDK was not used in any of the modules.
As part of the PR, I have addressed various deprecations in the NodeJS environment and related required changes in JS tests.