Satisfy minimum requirements for full typescript project #37

FranjoMindek · 2025-03-21T08:11:22Z

This is satisfying all tsc and eslint requirements when we convert each of the js files to ts.
Rest of changes will be in further PRs

Contains:

import changes.
logger.ts has been fully changed, due to using invalid constructor syntax.
bugfix on arrays on discord-bot.ts
let -> const where no reference change
miniumum types needed to typescript doesn't complain
handle nullable fields properly in cli.ts

src/analytics/charts.ts

src/analytics/cli.ts

src/analytics/reports/index.ts

Martinsos

Ok we are almost there I think!

src/analytics/cli.ts

src/analytics/reports/index.ts

src/analytics/reports/periodReport/cohortRetentionReport.ts

src/analytics/reports/periodReport/index.ts

src/analytics/reports/periodReport/projectsReport.ts

src/analytics/reports/totalReport.ts

src/discord-bot.ts

Martinsos

Nice! Left some comments, I think they are quite clear, and I will let the @sodic take over the PR review and do the final accept, since we are getting more into TS types and he knows this stuff better. I will be lurking so I can see the final result and learn.

src/analytics/reports/totalReport.ts

src/analytics/reports/periodReport/cohortRetentionReport.ts

Martinsos

I am giving a neutral "comment" to remove my "request changes", so that @sodic can do the final approve when he finds it fit.

src/analytics/cli.ts

Martinsos · 2025-03-25T12:37:37Z

src/analytics/reports/periodReport/index.ts

@@ -12,19 +11,44 @@ import { generateCohortRetentionReport } from "./cohortRetentionReport";
 // they are prepared (our events removed, sorted) and that they are all events available for CLI,
 // for the whole history. You should obtain them with fetchAllCliEvents(), in that case they will
 // be all good.
-export async function generatePeriodReport(
+export async function generateFullPeriodReport(


What about typing these guys also? Or are we leaving this for later?

I can do that now.

src/discord-bot.ts

Martinsos · 2025-03-25T12:47:40Z

src/analytics/reports/index.ts

+
+type ReportName = string;
+
+export type SimpleReport = {


If I am correct, we have these individual report files importing from this index file, because they need SImpleReport, but then we also have this file importing from them because it is using them. So we have a cyclic dependency. I know JS can suffer those to a degree, but still, might be good to avoid it. Therefore, I would consider extracting SimpleReport into separate file, simpleReport.ts .

I would also restructure then.
We have maybe too much files in the reports folder, and also the fact that both simpleReport and totalReport are located in the same reports directory. The naming indicates that they are similar, but one is only a type while the other is an actual report. The current structure is a bit weird. I wanted to do that in next PR but we could change it here.

I don't really mind that one has type and another does not though, types and functions are all important. Ideally files would be organized by domain. I would break the circ dep here now immediately, and then in the next PR you can try to improve the structure further if you wish.

I'll just wait for @sodic to give his opinion on types (they might change completely), and after that, if it's still a circ dep I will break it.

I figured out what's bothering me here.

Partial<SimpleReport> is a supertype of all reports, but its name suggests otherwise. And pure SimpleReport isn't use anywhere except to define other types.

SimpleReport's name is derived from CompositeReport (i.e., because there's a composite thing, this is simple thing)

The relationship is defined backwards - it's the printing that requires this interface, not the reports.

The string in [reportName: string] should be bounded. The entire type is probably underspecified.

This is a sketch of how I'd expect the type definitions to look like:

export type GenericReport = Partial<{ text: string[]; csv: (number | string)[][]; chart: ImageCharts; }>; export type CompositeReport = PeriodReport | TotalReport; type TotalReport = { totalReport: Pick<GenericReport, "text">; } type PeriodReport = { cohortRetentionReport: CohortRetentionReport; userActivityReport: UserActivityReport; projectsReport: ProjectsReport; }; export type CohortRetentionReport = Pick<GenericReport, "text">; export type UserActivityReport = Pick<GenericReport, "text" | "csv" | "chart">; export type ProjectsReport = Pick<GenericReport, "text" | "csv">;

I might have missed something as I'm still not 100% familiar with the code, but these types convey an explicit structure and tell a story. Ideally we'd also colocate them.

@FranjoMindek Try to work this in and let me know if you need help. We can have a call tomorrow (today :))

Martinsos

@FranjoMindek did a bit more of reviewing!

sodic

Nice work! Left some comments, will come back with more probably :)

sodic · 2025-03-26T11:20:07Z

src/analytics/charts.ts

+import ImageCharts from "image-charts";
+
+export type ChartData = {
+  series: Record<string, number[]>;


Just FYI, no need to change. Read this and pick your preference: wasp-lang/wasp#1235 (comment)

I like the idea of having different intents with records and index signatures. Though, for that to work the whole team must be educated on it. Personally, I like it.

Glad to hear it! Time for an inquisition then :)

You put in a rocket here but didn't change it. Forgot to push something?

sodic · 2025-03-26T11:29:38Z

src/analytics/charts.ts


 // Expects data to be:
 //   data = { series: { name: [number] }, periodEnds: [string] }
 // Returns a string, URL leading to image-charts.com that contains query with exact
 // instructions how to display this chart via image.
-export function buildChartImageUrl(data, title, type = "line") {
-  const chart = ImageCharts()
+export function buildChartImageUrl(


I know we're typing this in the next PR, so no changes necessary here.

Still, FYI: wasp-lang/vscode-wasp#6 (comment)

src/analytics/reports/periodReport/index.ts

src/analytics/cli.ts

sodic · 2025-03-26T11:56:48Z

src/analytics/reports/index.ts

 export { fetchEventsForReportGenerator } from "./events";
 export { generateTotalReport } from "./totalReport";
-import { generatePeriodReport } from "./periodReport";
+
+type ReportName = string;


Unbranded type aliases can be avoided/improved in most cases. This is usually desriable because aliases can give a false sense of type safety and obfuscate primitive types.

In this case, it's used to name a Record's key. A better way to do this is an index signature:

export type CompositeReport = { [reportName: string]: Partial<SimpleReport> };

Further reference: wasp-lang/wasp#1235 (comment)

Apologies, I forgot to mention this in writing.

I have a strong feeling the key type should be a bounded subset of string. Is that right?

src/analytics/reports/index.ts

src/analytics/reports/periodReport/cohortRetentionReport.ts

sodic

Finally got around to it :)

sodic · 2025-03-31T23:07:54Z

src/analytics/reports/periodReport/index.ts

+type BasePeriodReport = {
+  userActivityReport: UserActivityReport;
+  projectsReport: ProjectsReport;
+};
+
+export type AllTimePeriodReort = BasePeriodReport;
+
+export type PeriodReport = BasePeriodReport & {
+  cohortRetentionReport: CohortRetentionReport;
+};


What's going on here, why is the all time report different than the period report?

Also, the names seem to off:

AllTimePeriodReport sounds like a specialization of a PeriodReport but it isn't, meaning that PeriodReport is also missing some kind of a prefix (or that AllTimePeriodReport shouldn't have Period in it)

After fixing that, I believe the Base prefix on BasePeriodReport becomes unnecessary.

That is a bit weird relationship.

PeriodReport is a general report over a period of time.

AllTimePeriodReport is ?variation? of PeriodReport which excludes CohortRetentionReport because of it's complexity. This is because a period report which spans the whole timeline has too much data.

Maybe PeriodReport and AllTimeReport?

If it's true that they share part of reports by accident (they both relate to the time domain) but are actually different, it would be best to extract AllTimeReport to a different file?

Now that I look at it TotalReport also covers the whole period of time. There is again some overlap.

Report restructuring was decided as separate PR

sodic · 2025-03-31T23:09:41Z

src/analytics/reports/index.ts

 export { fetchEventsForReportGenerator } from "./events";
 export { generateTotalReport } from "./totalReport";
-import { generatePeriodReport } from "./periodReport";
+
+type ReportName = string;


Apologies, I forgot to mention this in writing.

I have a strong feeling the key type should be a bounded subset of string. Is that right?

sodic · 2025-03-31T23:26:39Z

src/analytics/reports/index.ts

+
+type ReportName = string;
+
+export type SimpleReport = {


I figured out what's bothering me here.

Partial<SimpleReport> is a supertype of all reports, but its name suggests otherwise. And pure SimpleReport isn't use anywhere except to define other types.

SimpleReport's name is derived from CompositeReport (i.e., because there's a composite thing, this is simple thing)

The relationship is defined backwards - it's the printing that requires this interface, not the reports.

The string in [reportName: string] should be bounded. The entire type is probably underspecified.

This is a sketch of how I'd expect the type definitions to look like:

export type GenericReport = Partial<{ text: string[]; csv: (number | string)[][]; chart: ImageCharts; }>; export type CompositeReport = PeriodReport | TotalReport; type TotalReport = { totalReport: Pick<GenericReport, "text">; } type PeriodReport = { cohortRetentionReport: CohortRetentionReport; userActivityReport: UserActivityReport; projectsReport: ProjectsReport; }; export type CohortRetentionReport = Pick<GenericReport, "text">; export type UserActivityReport = Pick<GenericReport, "text" | "csv" | "chart">; export type ProjectsReport = Pick<GenericReport, "text" | "csv">;

I might have missed something as I'm still not 100% familiar with the code, but these types convey an explicit structure and tell a story. Ideally we'd also colocate them.

@FranjoMindek Try to work this in and let me know if you need help. We can have a call tomorrow (today :))

sodic · 2025-03-31T23:32:24Z

src/analytics/reports/periodReport/index.ts

+  projectsReport: ProjectsReport;
+};
+
+export type AllTimePeriodReort = BasePeriodReport;


This guy has a typo.

sodic · 2025-03-31T23:40:14Z

src/analytics/reports/periodReport/index.ts

+): Promise<PeriodReport> {
+  const events = prefetchedEvents ?? (await fetchEventsForReportGenerator());
+
+  const basePeriodReeport = await generatePeriodReportBaseReports(


Typo in "reeport"

FranjoMindek · 2025-04-01T09:32:17Z

The problem of

export type CompositeReport = PeriodReport | TotalReport | ...;

Is that when we have a function which accepts CompositeReport, if we want to do generic operations on values of any of composite report's reports, it will automatically restrict them to the intersection between all of the composite reports components.

In the end: Object.values(compositeReport) will return { text: string; }.
If in the future we create a simple report without text field, it will return any.
You will need to assert them to GenericReport.

~~They can be check for other fields ("text" in simpleReport) but intellisense won't tell you that they exist before you check for them yourself. You need to have that knowledge yourself.~~ Actually you can't even expand them like that.

src/discord-bot.ts

FranjoMindek · 2025-04-01T11:48:11Z

src/analytics/reports/reports.ts

+export interface TextReport {
+  text: string[];
+}
+


report specific functions will also go here

sodic

Looks good!

Don't forget to take care of the restructuring in a different PR :)

Feature: satisfy minimum for full typescript

dd80c4e

FranjoMindek self-assigned this Mar 21, 2025

Martinsos requested changes Mar 21, 2025

View reviewed changes

src/analytics/charts.ts Outdated Show resolved Hide resolved

src/analytics/cli.ts Outdated Show resolved Hide resolved

Feature: change report structure from array to object

d9c816f

FranjoMindek commented Mar 21, 2025

View reviewed changes

src/analytics/reports/index.ts Show resolved Hide resolved

FranjoMindek requested a review from Martinsos March 21, 2025 12:45

Chore: jsdoc

abc89cf

Martinsos reviewed Mar 21, 2025

View reviewed changes

Refactor: PR review suggestions

a2c62c4

Martinsos reviewed Mar 24, 2025

View reviewed changes

src/analytics/reports/totalReport.ts Show resolved Hide resolved

src/analytics/reports/periodReport/cohortRetentionReport.ts Show resolved Hide resolved

Refactor: type out simple reports

f9bd865

FranjoMindek requested a review from sodic March 24, 2025 16:39

Martinsos reviewed Mar 25, 2025

View reviewed changes

src/analytics/cli.ts Outdated Show resolved Hide resolved

Martinsos reviewed Mar 25, 2025

View reviewed changes

src/discord-bot.ts Outdated Show resolved Hide resolved

Martinsos reviewed Mar 25, 2025

View reviewed changes

Refactor: rename variables

f1790df

sodic reviewed Mar 26, 2025

View reviewed changes

FranjoMindek added 4 commits March 27, 2025 11:02

Refactor: more types for now some logic errors

c532fd4

Chore: docs

4dec0ed

Fix: type -> interface

fd00212

Chore: comments

ea68f90

FranjoMindek requested a review from sodic March 28, 2025 15:00

sodic reviewed Mar 31, 2025

View reviewed changes

Type refactor

a7a320a

FranjoMindek commented Apr 1, 2025

View reviewed changes

src/discord-bot.ts Outdated Show resolved Hide resolved

FranjoMindek commented Apr 1, 2025

View reviewed changes

FranjoMindek added 4 commits April 1, 2025 14:14

Improve report to discord message

00ccbef

Make it optional

ddf10ac

Improve discord message creation

4bbbec0

renames

3a8807e

FranjoMindek requested a review from sodic April 7, 2025 07:34

sodic approved these changes Apr 7, 2025

View reviewed changes

Record to index

99a02b7

FranjoMindek merged commit a29ed01 into master Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Satisfy minimum requirements for full typescript project #37

Satisfy minimum requirements for full typescript project #37

FranjoMindek commented Mar 21, 2025 •

edited

Loading

Martinsos left a comment

Martinsos left a comment

Martinsos left a comment

Martinsos Mar 25, 2025

FranjoMindek Mar 25, 2025

Martinsos Mar 25, 2025

FranjoMindek Mar 25, 2025

Martinsos Mar 26, 2025

FranjoMindek Mar 27, 2025

sodic Mar 31, 2025

Martinsos left a comment

sodic left a comment

sodic Mar 26, 2025

FranjoMindek Mar 27, 2025

sodic Mar 31, 2025

sodic Mar 31, 2025

sodic Mar 26, 2025

sodic Mar 26, 2025

sodic Mar 31, 2025

sodic left a comment

sodic Mar 31, 2025 •

edited

Loading

FranjoMindek Apr 1, 2025

FranjoMindek Apr 1, 2025

FranjoMindek Apr 7, 2025

sodic Mar 31, 2025

sodic Mar 31, 2025

sodic Mar 31, 2025

sodic Mar 31, 2025

FranjoMindek commented Apr 1, 2025

FranjoMindek Apr 1, 2025

sodic left a comment

Satisfy minimum requirements for full typescript project #37

Satisfy minimum requirements for full typescript project #37

Conversation

FranjoMindek commented Mar 21, 2025 • edited Loading

Martinsos left a comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

sodic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sodic left a comment

Choose a reason for hiding this comment

sodic Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranjoMindek commented Apr 1, 2025

Choose a reason for hiding this comment

sodic left a comment

Choose a reason for hiding this comment

FranjoMindek commented Mar 21, 2025 •

edited

Loading

sodic Mar 31, 2025 •

edited

Loading