Bdomen react api utils

[bdomen-ggl]

Adding the UI models and API code.
Notes:

• Not hooking up to the backend yet (BFF not yet implemented) so using a Fake Api.
• Can easily switch to the Real Api (and finish implementing it) when the BFF server is ready.
• Models are written in TypeScript to help with type safety in the UI. They are at the moment arbitrarily made to make the UI easier to render. The idea would be the BFF would emit a similar patter, but still experimenting with this.
• Starting to add some things like memoization. Not needed immediately with fake data, but will be needed when hooking up to the BFF. Can still use some enhancements before then.

[wfa-reviewable]

This change is 

[bdomen-ggl]

Reviewable status: 0 of 16 files reviewed, 5 unresolved discussions (waiting on @bdomen-ggl and @SanjayVas)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 18 at r2 (raw file):

export const memoizePromiseFn = fn => {
  return (...args) => {
    const key = JSON.stringify(args);

May want to add the function name or some other base key in addition to the args later.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/realApi.ts line 24 at r2 (raw file):

import path from 'path';

export class RealApi implements IReportingApi {

Just stubbing this for now until the BFF is ready for use.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/reportingApi.ts line 25 at r2 (raw file):

import {IReportingApi} from './IReportingApi';

export class ReportingApi {

This will ultimately be the class to be called. It will handle common tasks like memoization (below) and logging (future) so they don't have to be reimplemented with every implementation.

Alternatively this can be removed/replaced with the real version as we won't realistically have multiple implementations and can remove a layer of abstraction.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/reportingApi.ts line 39 at r2 (raw file):

  getReport(req: GetReportRequest): Promise<GetReportResponse> {
    const getCached = memoizePromiseFn(this.api.getReport);

Reports are immutable. If we ask for the same one, we'll always get the same result. This will be useful in the UI when multiple widgets will want to ultimately call Get Report. This will ensure the request is only made once over the wire.

We can add memoization to other methods like List Reports, but the results can change with the same request and adding a refresh feature is excessive at the moment. May add memoization with a refresh button in the future.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/models/InitApiProps.ts line 16 at r2 (raw file):

export type InitApiProps = {
  // eslint-disable-next-line node/no-unsupported-features/node-builtins

Working on updating the eslint configuration to allow this as it's supported in the environment we're using. Not high priority right now.

[SanjayVas]

Reviewed 6 of 16 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 10 of 16 files reviewed, 13 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

// Copyright 2023 The Cross-Media Measurement Authors

Fakes are test infrastructure, so they should go under a testing Bazel package with default_testonly = True to ensure that they can't be used from non-test code.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

// Copyright 2023 The Cross-Media Measurement Authors

I think file names should be using snake_case for consistent style.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 305 at r2 (raw file):

  async listReports(): Promise<ListReportsResponse> {
    return new Promise(resolve =>
      resolve({

Freeze the response object (anything representing a service response should be immutable).

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 308 at r2 (raw file):

        reports: this.reports,
      })
    );

Assuming this is a standard ECMAScript Promise, then you should be able to just use Promise.resolve() to build an immediately-resolved Promise.

Suggestion:

    return Promise.resolve({reports: this.reports});

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

// limitations under the License.

const cache = new Map();

I don't think I like having a single global cache for everything. Each cache should have its properties, e.g. max size and expiration.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/realApi.ts line 35 at r2 (raw file):

  async listReports(): Promise<ListReportsResponse> {
    return fetch(path.join(this.baseUrl.toString(), '/api/reports'))

The main benefit of using async functions is so you can use await within them rather than having to chain thens.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/reportingApi.ts line 39 at r2 (raw file):

Reports are immutable. If we ask for the same one, we'll always get the same result

If you're talking about the resource from the Reporting API, this is not true. Reports have state. A Report only stops being mutable once it reaches a terminal state.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/models/Demo.ts line 15 at r2 (raw file):

// limitations under the License.

export type Demo = {

nit: Be more verbose. Is this supposed to be a demographic?

Code quote:

Demo

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/IReportingApi.ts line 22 at r2 (raw file):

} from './models';

export interface IReportingApi {

Are we following https://google.github.io/styleguide/tsguide.html? If so, then this shouldn't have an I prefix (unless that's particularly idiomatic to React TS). See https://google.github.io/styleguide/tsguide.html#naming-style

[bdomen-ggl]

Reviewable status: 10 of 16 files reviewed, 13 unresolved discussions (waiting on @SanjayVas)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Fakes are test infrastructure, so they should go under a testing Bazel package with default_testonly = True to ensure that they can't be used from non-test code.

Sure. This is intended to be used so I can build out more of the UI service code while still being able to render data in the UI without hardcoding it since we don't have a backend server to hit yet.

Would you suggest I make "real" API calls instead (despite the backend doesn't exist yet) and visualize my views in another tool like Storybook? https://storybook.js.org/ I opted against this at first because we're trying to push out some UI code and setting up Storybook would be another piece that would take a while to add. Also, the nice thing about the fake data in the app vs something like Story Book is that I can go through user workflows (granted we only have two views) and observe how API calls might be optimized.

I could build out the backend server first but was asked to push a UI with fake data so I started with a Fake Api. We can revisit this discussion if you would prefer this route.

I could also just leave the Fake Api local only (not check it in), but would be worried I might accidentally delete it and have to re-write it.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think file names should be using snake_case for consistent style.

I can do that. Everywhere I've been at before used camel case for folders and pascal case for files in javascript/typescript projects so I just started with that. The idea being that the name of classes tend to be the same as the file name (some linting rules I've used enforced this).

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 305 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Freeze the response object (anything representing a service response should be immutable).

I can do this, but freeze is only a shallow freeze and there's lots of nested objects.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

Are you also suggesting I recursively go through every property that is an object and freeze them before assigning? Or is the shallow freeze good enough?

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 308 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Assuming this is a standard ECMAScript Promise, then you should be able to just use Promise.resolve() to build an immediately-resolved Promise.

Yes I can. I was initially doing it this way since I was expecting there to be some more logic. I simplified some assumptions so that's no longer the case.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I don't think I like having a single global cache for everything. Each cache should have its properties, e.g. max size and expiration.

That will probably be true. There's only one use with Get Report right now. I was going to wait until we needed to use this more to see what the requirements will be before adding more complexity. It's technically not even a cache (in the sense of a fully featured cache) since it's just a map.

I can change it to a cache and make it specific to the caller to fine tune caching policies. I'll likely hardcode the policies for now and add an app config in a future PR. I'll need other things anyways like Reporting BFF API URL.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/realApi.ts line 35 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The main benefit of using async functions is so you can use await within them rather than having to chain thens.

Yes. Either async/await or just using thens is fine. I copied fetch from somewhere and didn't update the thens. There's some subtle differences between the two but I don't think they really matter in this case.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/reportingApi.ts line 39 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Reports are immutable. If we ask for the same one, we'll always get the same result

If you're talking about the resource from the Reporting API, this is not true. Reports have state. A Report only stops being mutable once it reaches a terminal state.

Ah yes that's technically right. The non-terminal state will likely lend itself to another view since all the charts and graphs won't have anything. I'll take a look at this case.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/models/Demo.ts line 15 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: Be more verbose. Is this supposed to be a demographic?

Yes, I can change the name.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/IReportingApi.ts line 22 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Are we following https://google.github.io/styleguide/tsguide.html? If so, then this shouldn't have an I prefix (unless that's particularly idiomatic to React TS). See https://google.github.io/styleguide/tsguide.html#naming-style

I threw this up last second and copied an existing example I saw. I can switch it to the style guide you linked.

[SanjayVas]

Reviewable status: 10 of 16 files reviewed, 13 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, bdomen-ggl wrote…

Sure. This is intended to be used so I can build out more of the UI service code while still being able to render data in the UI without hardcoding it since we don't have a backend server to hit yet.

Would you suggest I make "real" API calls instead (despite the backend doesn't exist yet) and visualize my views in another tool like Storybook? https://storybook.js.org/ I opted against this at first because we're trying to push out some UI code and setting up Storybook would be another piece that would take a while to add. Also, the nice thing about the fake data in the app vs something like Story Book is that I can go through user workflows (granted we only have two views) and observe how API calls might be optimized.

I could build out the backend server first but was asked to push a UI with fake data so I started with a Fake Api. We can revisit this discussion if you would prefer this route.

I could also just leave the Fake Api local only (not check it in), but would be worried I might accidentally delete it and have to re-write it.

I'm not saying the fake is a bad idea. You can still use it, just make sure it's in the right place.

The way we do this in server-side code is to have all service clients/stubs be provided to the top-level constructor. This way something at the top can decide whether to use the fake. I don't know what the top level is for a React app (is there some App type?) but you'd have a testing version of that which also goes under a testing Bazel package.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, bdomen-ggl wrote…

I can do that. Everywhere I've been at before used camel case for folders and pascal case for files in javascript/typescript projects so I just started with that. The idea being that the name of classes tend to be the same as the file name (some linting rules I've used enforced this).

This is coming from the Google JS style guide. It doesn't appear to be mentioned in the TS style guide, but I assume falling back to the JS style guide makes sense when the TS one doesn't specify.

If we're looking for other style guides, it looks like React is relatively unopinionated on the matter and there isn't a popular style guide that covers this. FWIW the Angular style guide uses lowercase with hyphens, so either way I don't think lowerCamelCase is the right choice.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 305 at r2 (raw file):

Previously, bdomen-ggl wrote…

I can do this, but freeze is only a shallow freeze and there's lots of nested objects.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

Are you also suggesting I recursively go through every property that is an object and freeze them before assigning? Or is the shallow freeze good enough?

Shallow is fine. It's mostly about intent, since JS can't really stop you from doing most things.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

Previously, bdomen-ggl wrote…

That will probably be true. There's only one use with Get Report right now. I was going to wait until we needed to use this more to see what the requirements will be before adding more complexity. It's technically not even a cache (in the sense of a fully featured cache) since it's just a map.

I can change it to a cache and make it specific to the caller to fine tune caching policies. I'll likely hardcode the policies for now and add an app config in a future PR. I'll need other things anyways like Reporting BFF API URL.

A map is a fine implementation of a cache (not every cache needs to handle all the policies/features internally). I also know that you were trying to share caching across the real and fake service clients, but I don't think that's necessary.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/IReportingApi.ts line 22 at r2 (raw file):

Previously, bdomen-ggl wrote…

I threw this up last second and copied an existing example I saw. I can switch it to the style guide you linked.

Maybe also call it ReportingClient instead, since it's really an API client not an API. I also don't think it makes sense to be in a utils package, as generally utils are free functions.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/models/Demo.ts line 15 at r2 (raw file):

Previously, bdomen-ggl wrote…

Yes, I can change the name.

Done

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/reportingApi.ts line 39 at r2 (raw file):

Previously, bdomen-ggl wrote…

Ah yes that's technically right. The non-terminal state will likely lend itself to another view since all the charts and graphs won't have anything. I'll take a look at this case.

Done

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/realApi.ts line 35 at r2 (raw file):

Previously, bdomen-ggl wrote…

Yes. Either async/await or just using thens is fine. I copied fetch from somewhere and didn't update the thens. There's some subtle differences between the two but I don't think they really matter in this case.

Done
Removed async since this is only doing a fetch and I want to return a promise anyways. This allows the API code to be called in a UI in case someone wants to access it directly instead of through the helpers.

Specifically, someone could call the API directly in a React component using useMemo (React built-in memoization method, but only for a React component). Didn't use this because we'd still need one lower down the call stack.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/realApi.ts line 35 at r2 (raw file):

Previously, bdomen-ggl wrote…

Done
Removed async since this is only doing a fetch and I want to return a promise anyways. This allows the API code to be called in a UI in case someone wants to access it directly instead of through the helpers.

Specifically, someone could call the API directly in a React component using useMemo (React built-in memoization method, but only for a React component). Didn't use this because we'd still need one lower down the call stack.

Actually, left async/await and removed thens. It was an autosuggestion and I didn't realize how that behavior worked.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

A map is a fine implementation of a cache (not every cache needs to handle all the policies/features internally). I also know that you were trying to share caching across the real and fake service clients, but I don't think that's necessary.

DONE
Gotcha. I moved this all into a class so it's not global anymore. And yeah, I was trying to share the caching. Since the widgets are independent of each other, they can make many of the same calls to Get Report.

Also, I looked into the cache API, but it must use URLs as keys, which I don't think we necessarily want to enforce since there may be other operations that would be useful to memoize as well. Also when using the fake data class, there isn't a URL being called.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/IReportingApi.ts line 22 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Maybe also call it ReportingClient instead, since it's really an API client not an API. I also don't think it makes sense to be in a utils package, as generally utils are free functions.

Done
Yeah, that makes sense. I was having a rough time with naming this one since there's similar things.
Renamed to ReportingClient and moved everything out of utils.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 308 at r2 (raw file):

Previously, bdomen-ggl wrote…

Yes I can. I was initially doing it this way since I was expecting there to be some more logic. I simplified some assumptions so that's no longer the case.

Done

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 305 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Shallow is fine. It's mostly about intent, since JS can't really stop you from doing most things.

Done
Sounds good. Just wanted to make sure.

[bdomen-ggl]

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is coming from the Google JS style guide. It doesn't appear to be mentioned in the TS style guide, but I assume falling back to the JS style guide makes sense when the TS one doesn't specify.

If we're looking for other style guides, it looks like React is relatively unopinionated on the matter and there isn't a popular style guide that covers this. FWIW the Angular style guide uses lowercase with hyphens, so either way I don't think lowerCamelCase is the right choice.

Done
React is fairly un-opinionated on the topic. They do have some base suggestions from the create react app repo (nothing written, just how the app is created). It's also confusing because they have a different set of styles for JS vs TS. I'll stick with the style guide you linked.

[SanjayVas]

Reviewed 9 of 22 files at r3, all commit messages.
Reviewable status: 9 of 22 files reviewed, 8 unresolved discussions (waiting on @bdomen-ggl)

a discussion (no related file):
This is missing BUILD.bazel files. Each folder with source files should be a Bazel package and therefore have a BUILD.bazel file. A source file should generally not be referenced outside of a Bazel package directly, and instead should be accessed via an appropriate target (usually using a <lang>_library rule.

Bazel packages (meaning folder names) should also all be lowercase.

---

---

experimental/reporting-ui/src/main/react/reporting-ui/library/reportingClient/real_api.ts line 36 at r3 (raw file):

  async listReports(): Promise<ListReportsResponse> {
    const res = await fetch(path.join(this.baseUrl.toString(), '/api/reports'));
    const reports = await res.json();

I don't think this returns a Promise. See https://developer.mozilla.org/en-US/docs/Web/API/Response/json_static

Suggestion:

res.json();

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

Gotcha. I moved this all into a class so it's not global anymore. And yeah, I was trying to share the caching.

Sorry, the "global" part I was concerned with wasn't the fact that the functions were unscoped (though that too), but that the cache is shared. If you're trying to cache RPC responses, that should be handled in the client impl.

Since the widgets are independent of each other, they can make many of the same calls to Get Report.

That doesn't seem right. AFAIK all of the common MV* design patterns use model objects. Some single thing (e.g. a button) should trigger loading of a Report, e.g. updating or replacing some Report model object. How that gets propagated to all the components is dependent on the pattern.

[bdomen-ggl]

Reviewable status: 7 of 30 files reviewed, 8 unresolved discussions (waiting on @SanjayVas)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is missing BUILD.bazel files. Each folder with source files should be a Bazel package and therefore have a BUILD.bazel file. A source file should generally not be referenced outside of a Bazel package directly, and instead should be accessed via an appropriate target (usually using a <lang>_library rule.

Bazel packages (meaning folder names) should also all be lowercase.

Right, I was going to add it later when the library starts to get used as I add the views. I can add BUILD.bazel now.

By the way, when you say "each folder with source files," do you mean ALL folders including all subdirectories or just the top level ones? I was generally following the Aspect example here (https://github.com/bazelbuild/rules_nodejs/tree/stable/examples/create-react-app) and they only have the BUILD file at the workspace root. Although they're building in a very different way using react_scripts. I thought it would be nice to go another level and add BUILD files for things like src and public since they contain different kinds of files, like they could be different packages.

UPDATE: Talked with Sanjay. The example I was following is a bad Bazel pattern (using a glob to load "**/*.ts" for example) and I should not do it that way. And yes, each and every folder should have a BUILD.bazel file. Got the library/BUILD file, but need to add all the rest and update the first one to NOT use the bad pattern and reference the subdirectories.

---

---

experimental/reporting-ui/src/main/react/reporting-ui/library/reportingClient/real_api.ts line 36 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I don't think this returns a Promise. See https://developer.mozilla.org/en-US/docs/Web/API/Response/json_static

Ah weird.
What I had was the suggestion from the fetch API docs:
https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch.
Mouseover in my environment also shows .json() returns a promise. Same when I use a JavaScript playground online. I'll look into this some more.
It does seem weird that .json() would return a promise.

UPDATE: Ah nvm, you are looking at the static method Response.json(obj) but I'm using the instance method response.json(). The prior returns an object and the later returns a promise.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/fakeApi.ts line 1 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'm not saying the fake is a bad idea. You can still use it, just make sure it's in the right place.

The way we do this in server-side code is to have all service clients/stubs be provided to the top-level constructor. This way something at the top can decide whether to use the fake. I don't know what the top level is for a React app (is there some App type?) but you'd have a testing version of that which also goes under a testing Bazel package.

WIP
React kind of does but kind of doesn't have a top-level constructor. There's the index.tsx which is the entry point for webpack bundling where we can probably add configurations and the like. I'm having an annoying time since JavaScript doesn't handle DI terribly nicely. So trying to find a way to inject even like a class map at the top level.

UPDATE: Going to copy index.tsx to the test folder and update the imports to the main folder. Then before the app gets rendered, I can bootstrap the appropriate configurations (dev/test/prod) including setting up the reporting client.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/utils/api/memoize.ts line 15 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Gotcha. I moved this all into a class so it's not global anymore. And yeah, I was trying to share the caching.

Sorry, the "global" part I was concerned with wasn't the fact that the functions were unscoped (though that too), but that the cache is shared. If you're trying to cache RPC responses, that should be handled in the client impl.

Since the widgets are independent of each other, they can make many of the same calls to Get Report.

That doesn't seem right. AFAIK all of the common MV* design patterns use model objects. Some single thing (e.g. a button) should trigger loading of a Report, e.g. updating or replacing some Report model object. How that gets propagated to all the components is dependent on the pattern.

Talked on a call about the MV[something] pattern:
I was under the impression we wanted to have 100% self-contained widgets (they would know how to render and fetch data given a base reporting api url) and they would be their own MV[something] pattern. This is not the desired behavior. The upper level "something" (ie. the get report view business logic) should handle fetching the data, turning it into the right model (if not automatically done by the api) and then passed to the widget that will just render the model based on it's template. The widget should not know how to populate its own data.

Don't think there's much else to change in this PR since it's only memoizing the call to the reporting api.

[SanjayVas]

Reviewed all commit messages.
Reviewable status: 0 of 35 files reviewed, 12 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 1 at r5 (raw file):

load("@aspect_rules_js//js:defs.bzl", "js_library")

It seems odd to call this package library when most Bazel packages will expose library targets. What is it a library of?

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 15 at r5 (raw file):

]

js_library(

What's the benefit to having both a js_library and a ts_project (assuming the latter is also a library rule)? It looks like ts_library can take in TS source files, and that the js_library target isn't use outside of the package anyway (and should therefore have private visibility).

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 22 at r5 (raw file):

)

ts_project(

So is ts_project effectively a library rule?

For background, most Bazel packages tend to expose <lang>_library targets for that language. Then there's a <lang>_binary target that does the compilation.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 23 at r5 (raw file):

ts_project(
    name = "src_ts",

Generally if you have a single library target for a Bazel package you name it the same thing as the package to indicate it's the "default" target.

Using more granular targets is preferred over globbing all source files in a package, but not necessary if all of the sources get used together anyway.

Code quote:

src_ts

[bdomen-ggl]

Reviewable status: 0 of 35 files reviewed, 12 unresolved discussions (waiting on @SanjayVas)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 1 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

It seems odd to call this package library when most Bazel packages will expose library targets. What is it a library of?

Didn't have a really good name for it but the idea is that this where the code will be that we can package up later to make an external library if desired. The idea being any code within this folder should not need to depend on any code outside of it.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 15 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

What's the benefit to having both a js_library and a ts_project (assuming the latter is also a library rule)? It looks like ts_library can take in TS source files, and that the js_library target isn't use outside of the package anyway (and should therefore have private visibility).

I'll go double check all of them. Before switching to the webpack bazel rule for sure and perhaps other rules I was trying, they required js_library. The webpack bazel rule I ended up using can consume ts_project as deps using webpack_bundle or data using webpack_devserver. So like you said, most if not all of these can probably be private to the package.

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 22 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

So is ts_project effectively a library rule?

For background, most Bazel packages tend to expose <lang>_library targets for that language. Then there's a <lang>_binary target that does the compilation.

ts_project wraps the tsc cli:
https://github.com/aspect-build/rules_ts/blob/main/docs/rules.md

If you're suggesting to then take that output and put it in a js_library or something I can do that with this:
https://github.com/aspect-build/rules_js/blob/main/docs/js_library.md

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 23 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Generally if you have a single library target for a Bazel package you name it the same thing as the package to indicate it's the "default" target.

Using more granular targets is preferred over globbing all source files in a package, but not necessary if all of the sources get used together anyway.

That makes sense. I'll double check and update where needed.

I can update the file list to be more specific. Was thinking since a lot might be added/removed/renamed that it would be less overhead to use the glob and regex. Will do this for every BUILD file.

[SanjayVas]

Reviewable status: 0 of 35 files reviewed, 12 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 22 at r5 (raw file):

Previously, bdomen-ggl wrote…

ts_project wraps the tsc cli:
https://github.com/aspect-build/rules_ts/blob/main/docs/rules.md

If you're suggesting to then take that output and put it in a js_library or something I can do that with this:
https://github.com/aspect-build/rules_js/blob/main/docs/js_library.md

I'm just trying to figure out the intended usage, meaning what rules are intended to consume ts_project output. Looking at the inputs/outputs, it looks like this is meant to take in TS (and optionally JS) source files as srcs, and other targets that produce TS typings as deps. It outputs a JsInfo provider. Therefore it looks to me like a TS replacement for js_library.

[bdomen-ggl]

Reviewable status: 0 of 54 files reviewed, 13 unresolved discussions (waiting on @SanjayVas)

a discussion (no related file):
@SanjayVas Latest is ready. Updated the file structure and folder names and updated all the BUILD files.

---

---

experimental/reporting-ui/src/main/react/reporting-ui/library/BUILD.bazel line 22 at r5 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'm just trying to figure out the intended usage, meaning what rules are intended to consume ts_project output. Looking at the inputs/outputs, it looks like this is meant to take in TS (and optionally JS) source files as srcs, and other targets that produce TS typings as deps. It outputs a JsInfo provider. Therefore it looks to me like a TS replacement for js_library.

Talked off line. ts_project was an artifact from some build rules I realized I'm not longer using. Can just get rid of it. But yes, it can take TS and JS files as input and will output something similar to a js_library, but with any TS files compiled into JS.

[SanjayVas]

Reviewed 1 of 11 files at r4, 1 of 31 files at r5, 3 of 15 files at r6, 45 of 49 files at r7, all commit messages.
Reviewable status: 50 of 54 files reviewed, 13 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/BUILD.bazel line 8 at r7 (raw file):

js_library(
    name = "src",

Prefer more descriptive names.

Suggestion:

app

---

experimental/reporting-ui/src/main/react/reporting-ui/BUILD.bazel line 24 at r7 (raw file):

js_library(
    name = "build_node_deps",

Not sure what the "build" is about. Perhaps just node_deps or node_modules?

Suggestion:

node_modules

---

experimental/reporting-ui/src/test/react/reporting-ui/fake/src/BUILD.bazel line 9 at r7 (raw file):

)

js_library(

This should be marked testonly and in a testing package. reporting-ui/client/reporting/testing seems like a good place, since this is a fake implementation of ReportingClient for testing.

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/client_impl.ts line 33 at r7 (raw file):

  getReport(req: GetReportRequest): Promise<GetReportResponse> {
    const getCached = this.memoizer.memoizePromiseFn(this.api.getReport);

1. Any caching should be specific to the implementation rather than attempting to share it across implementations (e.g. a real implementation may want to cache, a fake won't).
2. It's only safe to permanently cache Reports in terminal states, meaning caching here can't be implemented as simple memoization.

This means that you shouldn't have a base ReportingClientImpl that's shared across implementations, and you can freely use that name for the real client impl.

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/real_api.ts line 23 at r7 (raw file):

import {ReportingClient} from './client';

export class RealApi implements ReportingClient {

The real client impl can just be named ReportingClientImpl.

See comment on client_impl.ts

Code quote:

RealApi

[bdomen-ggl]

Reviewable status: 39 of 55 files reviewed, 13 unresolved discussions (waiting on @SanjayVas)

a discussion (no related file):

Previously, bdomen-ggl wrote…

@SanjayVas Latest is ready. Updated the file structure and folder names and updated all the BUILD files.

Next round of comments addressed.

---

---

experimental/reporting-ui/src/main/react/reporting-ui/BUILD.bazel line 8 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Prefer more descriptive names.

Done

---

experimental/reporting-ui/src/main/react/reporting-ui/BUILD.bazel line 24 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Not sure what the "build" is about. Perhaps just node_deps or node_modules?

Done

---

experimental/reporting-ui/src/test/react/reporting-ui/fake/src/BUILD.bazel line 9 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should be marked testonly and in a testing package. reporting-ui/client/reporting/testing seems like a good place, since this is a fake implementation of ReportingClient for testing.

Ah sorry. Forgot to mark testonly. And gotcha, thought when you said "testing package" before to put it in the test folder.

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/client_impl.ts line 33 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

1. Any caching should be specific to the implementation rather than attempting to share it across implementations (e.g. a real implementation may want to cache, a fake won't).
2. It's only safe to permanently cache Reports in terminal states, meaning caching here can't be implemented as simple memoization.

This means that you shouldn't have a base ReportingClientImpl that's shared across implementations, and you can freely use that name for the real client impl.

Done

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/real_api.ts line 23 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The real client impl can just be named ReportingClientImpl.

See comment on client_impl.ts

Done

[SanjayVas]

Reviewed 3 of 49 files at r7, 13 of 13 files at r8, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @bdomen-ggl)

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/client_impl.ts line 33 at r7 (raw file):

Previously, bdomen-ggl wrote…

Done

I still see this doing unconditional caching.

---

experimental/reporting-ui/src/main/react/reporting-ui/client/reporting/testing/fake_api.ts line 45 at r8 (raw file):

}

export class FakeApi implements ReportingClient {

Suggestion:

FakeReportingClient