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

TypeDoc #2092

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

TypeDoc #2092

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,7 @@ yarn-error.log*
.idea

# typescript
tsconfig.tsbuildinfo
tsconfig.tsbuildinfo

# typedoc
/typedoc/
9 changes: 9 additions & 0 deletions docs/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export { type RemoteItem, type RemoteList } from 'utils/storeUtils';
export {
loadItemIfNecessary,
loadListIfNecessary,
} from 'core/caching/cacheUtils';
export { default as shouldLoad } from 'core/caching/shouldLoad';
export { default as ZUIFuture, type ZUIFutureProps } from 'zui/ZUIFuture';
export { type IFuture } from 'core/caching/futures';
export { removeOffset } from 'utils/dateUtils';
32 changes: 32 additions & 0 deletions docs/testing/jest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
title: Jest
category: Testing
---

# Jest

## Running Jest

### Run all tests

```
yarn test
```

It can be useful to run the whole test suite if you've changed a few files as part of whatever you're working on.

### Run one test

```
yarn test src/utils/dateUtils.spec.ts
```

When you're working on one particular file, you can run its tests by putting the path to them after `yarn test`.

### Watch mode

```
yarn test --watch src/utils/dateUtils.spec.ts
```

During focused work on a single file, it can be helpful to use the `--watch` flag to re-run the tests automatically every time you change something.
77 changes: 77 additions & 0 deletions docs/testing/playwright.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
---
title: Playwright
category: Testing
---

# Playwright

## Running Playwright

### Run all tests

```
yarn playwright
```

It can be useful to run the whole playwright suite if you've changed a few features as part of whatever you're working on.

### Run one test

```
yarn playwright tests/organize/views/detail/display.spec.ts
```

When you're working on one particular feature, you can run its playwright tests by putting the path to them after `yarn playwright`.

### Skip the build

```
yarn playwright:skipbuild tests/organize/views/detail/display.spec.ts
```

The `yarn playwright` script builds the Next.js app before running the tests. This takes several minutes, and isn't useful if you're only changing test code (code in a `.spec.ts` file). You can tell playwright not to include this build step with the `playwright:skipbuild` script.

### Debug mode

```
yarn playwright:skipbuild --debug tests/organize/views/detail/display.spec.ts
```

In its default mode of execution, Playwright runs the tests in a headless browser. This means you can't see them running or interact with the browser's developer tools to debug any problems that arise. By adding the `--debug` flag, you can tell Playwright to run the tests visually in a Chrome window so you can see what's happening and debug any problems.

## Writing Playwright tests

### Moxy

Zetkins Playwright tests isolate the Next.js app from the back end very thoroughly by using [moxy](https://github.com/zetkin/moxy) to set up placeholder responses. This means the tests can run without the app ever actually communicating with the real back end. The purpose of this is to make the tests more deterministic, which means they should only break because of a problem with the code, because nobody can break them by mistake by changing something in the dev server's database that they depend on.

The [code search results for `moxy.setZetkinApiMock`](https://github.com/search?q=repo%3Azetkin%2Fapp.zetkin.org%20moxy.setZetkinApiMock&type=code) are a great starting point to learn about setting up these API mocks.

### waitForNavigation

A common step in a Playwright test is to trigger a click on a link and then continue after the page load has completed. The intuitive way to write this code would be like so.

```typescript
await page.click(`text=Next Page`);
await page.waitForNavigation();
```

You won't find any examples of code written that way in Zetkin though. The problem with writing it like that is that the navigation can sometimes complete before the `await page.waitForNavigation()` has even happened, leaving the test stranded waiting for a navigation that's already happened. Instead, we set up both steps simultaneously like this.

```typescript
await Promise.all([
page.waitForNavigation(),
page.click(`text=${AllMembers.title}`),
]);
```

### Locators

Browser automation tests like these are notorious for sometimes failing randomly. It's difficult to avoid making subtle mistakes and introducing race conditions like the `waitForNavigation` issue above. One general-purpose technique that can help with this is to prefer using [locators](https://playwright.dev/docs/locators) instead of `page.click()`. The first thing Playwright's own [documentation for `page.click`](https://playwright.dev/docs/api/class-page#page-click) says is not to use it.

> **Discouraged**<br />
> Use locator-based [locator.click()](https://playwright.dev/docs/api/class-locator#locator-click) instead. Read more about [locators](https://playwright.dev/docs/locators).

Using locators instead of `page.click()` maximizes our chances of Playwright's auto-waiting and auto-retrying saving us from a meaningless random test failure resulting from a race condition.

There are lots of examples to learn from in the [code search results for `page.locator`](https://github.com/search?q=repo%3Azetkin%2Fapp.zetkin.org%20page.locator&type=code).
18 changes: 18 additions & 0 deletions docs/time/time.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
title: Time
category: Time
---

# Time

## Day.js

Zetkin uses [Day.js](https://day.js.org/) to work with times and dates. Plugins used include [UTC](https://day.js.org/docs/en/plugin/utc), [IsoWeek](https://day.js.org/docs/en/plugin/iso-week), and [CustomParseFormat](https://day.js.org/docs/en/plugin/custom-parse-format).

## Serialization Format

The most commonly used date serialization format around the Zetkin front end is `2024-07-23T12:55:14.279Z`. A code search for [`new Date().toISOString()`](<https://github.com/search?q=repo%3Azetkin%2Fapp.zetkin.org%20%22new%20Date().toISOString()%22&type=code>) shows all the places where the current date & time are being serialized using this format by the front end code.

## Response Format

The Zetkin back end's resonses sometimes include dates serialized using a different format, with a `+00:00` suffix instead of the `Z` that the front end's `toISOString()` approach uses to denote the UTC timezone. For these, we use {@link removeOffset removeOffset} to strip the `+00:00` off the end.
Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

While not strictly wrong, this might be a bit misleading with regards to the "philosophy" of how we use time in Zetkin. Also, both the Z and +00:00 are valid ISO 8601 time strings and that's the standard that we try to adhere to, so it shouldn't matter if it's one or the other as long as it's valid by ISO 8601 and implemented in a way that understands ISO 8601.

I don't have a lot of time right now (no pun intended) but the short version of the Zetkin philosophy on timestamps is:

  1. Any timestamp generated on the server (e.g. to indicate when a thing was created/updated) should be UTC
  2. Any timestamp generated by the user (e.g. to schedule an activity) should allow any timezone

The hacks you see in a lot of places is because of the default behavior of JS Date (always parsing in local timezone) and (1) above, because although all server-generated timestamps are indeed UTC, a lot of them do not include timezone information, so that's added in the parsing step on the frontend.

The second part of the philosophy is quite new, and the only proper implementation of (2) so far is in email scheduling, where the UI actually presents the option to pick a timezone, and (barring any bugs) that timezone is included in the ISO timestamp as an offset between -12:00 and +12:00.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@
"ts-mockito": "^2.6.1",
"ts-node": "^10.9.1",
"tsconfig-paths-webpack-plugin": "^3.5.2",
"typedoc": "^0.26.5",
"typedoc-plugin-mermaid": "^1.12.0",
"typescript": "^5.4.3"
},
"engines": {
Expand Down
86 changes: 86 additions & 0 deletions src/core/caching/cacheUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,30 @@ import {
} from './futures';
import { RemoteItem, RemoteList } from 'utils/storeUtils';

/**
* Used by data fetching hooks to manage cache invalidation and fetching for their collection.
*
* A typical call to `loadListIfNecessary` looks like this one.
*
* ```typescript
* const tasksFuture = loadListIfNecessary(tasksList, dispatch, {
* actionOnLoad: () => tasksLoad(),
* actionOnSuccess: (data) => tasksLoaded(data),
* loader: () =>
* apiClient.get<ZetkinTask[]>(
* `/api/orgs/${orgId}/campaigns/${campId}/tasks`
* ),
* });
* ```
*
* Under the hood, {@link shouldLoad shouldLoad} is used for cache invalidation.
*
* @category Cache
* @param {RemoteList} remoteList The remote list to check and load.
* @param {AppDispatch} dispatch The Redux dispatch function.
* @param {Object} hooks Callbacks to handle the loading process.
* @return {IFuture} An {@link IFuture} object that can be used to render a loading spinner or the data itself.
*/
export function loadListIfNecessary<
DataType,
OnLoadPayload = void,
Expand All @@ -18,10 +42,35 @@ export function loadListIfNecessary<
remoteList: RemoteList<DataType> | undefined,
dispatch: AppDispatch,
hooks: {
/**
* Called when an error occurs while loading the list.
* @param err The error that occurred during the loading process.
* @return {PayloadAction} The action to dispatch when an error occurs.
*/
actionOnError?: (err: unknown) => PayloadAction<unknown>;

/**
* Called when the list begins loading.
* @returns {PayloadAction} The action to dispatch when the list is loading.
*/
actionOnLoad: () => PayloadAction<OnLoadPayload>;

/**
* Called when the list loads successfully.
* @returns {PayloadAction} The action to dispatch when the list has loaded.
*/
actionOnSuccess: (items: DataType[]) => PayloadAction<OnSuccessPayload>;

/**
* Optionally override {@link shouldLoad shouldLoad} with a custom function.
* @returns {boolean} Whether the list should be loaded.
*/
Comment on lines +64 to +67
Copy link
Member

@richardolsson richardolsson Sep 12, 2024

Choose a reason for hiding this comment

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

OMG I forgot about this. Here I am reading your documentation of my code and realizing it does things I didn't even know. 😊

isNecessary?: () => boolean;

/**
* The function that loads the list. Typically an API call.
* @returns {Promise<DataType[]>}
*/
loader: () => Promise<DataType[]>;
}
): IFuture<DataType[]> {
Expand Down Expand Up @@ -69,6 +118,29 @@ export function loadList<
return new PromiseFuture(promise);
}

/**
* Used by data fetching hooks to manage cache invalidation and fetching for their entity.
*
* A typical call to `loadItemIfNecessary` looks like this one.
*
* ```typescript
* const future = loadItemIfNecessary(submissionItem, dispatch, {
* actionOnLoad: () => submissionLoad(submissionId),
* actionOnSuccess: (data) => submissionLoaded(data),
* loader: () =>
* apiClient.get(`/api/orgs/${orgId}/survey_submissions/${submissionId}`),
* });
* ```
*
* Under the hood, {@link shouldLoad shouldLoad} is used for cache invalidation.
*
*
* @category Cache
* @param {RemoteItem} remoteItem The remote item to check and load.
* @param {AppDispatch} dispatch The Redux dispatch function.
* @param {Object} hooks Callbacks to handle the loading process.
* @return {IFuture} An {@link IFuture} object that can be used to render a loading spinner or the data itself.
*/
export function loadItemIfNecessary<
DataType,
OnLoadPayload = void,
Expand All @@ -77,8 +149,22 @@ export function loadItemIfNecessary<
remoteItem: RemoteItem<DataType> | undefined,
dispatch: AppDispatch,
hooks: {
/**
* Called when the item begins loading.
* @returns {PayloadAction} The action to dispatch when the item is loading.
*/
actionOnLoad: () => PayloadAction<OnLoadPayload>;

/**
* Called when the item loads successfully.
* @returns {PayloadAction} The action to dispatch when the item has loaded.
*/
actionOnSuccess: (item: DataType) => PayloadAction<OnSuccessPayload>;

/**
* The function that loads the item. Typically an API call.
* @returns {Promise<DataType[]>}
*/
loader: () => Promise<DataType>;
}
): IFuture<DataType> {
Expand Down
32 changes: 32 additions & 0 deletions src/core/caching/futures.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,40 @@
import { RemoteItem, RemoteList } from 'utils/storeUtils';

/**
* Encapsulates the state of an asynchronous operation to fetch a piece of data.
* Most commonly, this means a network request.
*
* ```mermaid
* flowchart TD
* A[data: null\nerror: null\nisLoading: false]
* B[data: null\nerror: null\nisLoading: true]
* C[data: null\nerror: Error\nisLoading: false]
* D[data: Object\nerror: null\nisLoading: false]
* E{Success?}
* A -->|Asynchronous operation begins| B
* B -->|Asynchronous operation ends| E
* E -->|Yes| D
* E -->|No| C
* ```
Comment on lines +7 to +18
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this should render a flowchart as some sort of graphic, but I'm not sure how to run the code in a way that does. I tried yarn typedoc which did build an output into ./typedoc/ that looked like something that I should be able to serve as static files, but that didn't work. What is the right way to view this?

Copy link
Member

Choose a reason for hiding this comment

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

An update. It seems that it was just really slow to load (note the loading duration in screenshot below), and one file failed:

image

After more than a minute of loading mermaid-related JS files, it finally rendered this:

image

Is this something that you are also experiencing or is it just me?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah not been very impressed with the reliability of typescript-plugin-mermaid tbh

*
*
* @category Async
*/
export interface IFuture<DataType> {
/**
* The payload of the asynchronous operation.
*/
data: DataType | null;

/**
* General purpose error object where any error that occurs during the
* asynchronous operation can be stored.
*/
error: unknown | null;

/**
* Denotes whether the operation is currently in progress.
*/
isLoading: boolean;
}

Expand Down
22 changes: 22 additions & 0 deletions src/core/caching/shouldLoad.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,35 @@ function isItem(
return 'deleted' in thing;
}

/**
* Determines if a remote object needs to be loaded from the server.
*
* Returns `false` for items that are deleted, currently loading, or have been
* loaded within the TTL.
*
* Returns `true` for items whose data is older than the TTL or has been marked
* as stale.
*
* @category Cache
* @param {RemoteItem|RemoteList} item The remote object to check.
* @return {boolean} `true` if the object should be loaded, `false` otherwise.
*/
export default function shouldLoad(
/**
* The remote object to check.
*/
item: RemoteItem<unknown> | RemoteList<unknown> | undefined
): boolean;
/**
* @category Cache
*/
export default function shouldLoad(
item: RemoteObjectRecord | undefined,
ids: number[]
): boolean;
/**
* @category Cache
*/
export default function shouldLoad(
item: ObjThatNeedsLoading,
idsOrVoid?: number[]
Expand Down
8 changes: 8 additions & 0 deletions src/utils/dateUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ export const getFirstAndLastEvent = (
*
* This is needed because event objects send an offset of +00:00 (UTC Time) from the server.
* It is not needed if the datetime string coming from the server doesn't contain an offset.
*
* ```typescript
* removeOffset("2000-01-01 00:00:00+00:00") // "2000-01-01 00:00:00"
* ```
*
* @category Time
* @param {string} datetime ISO datetime string with +00:00 offset
* @return {string} ISO datetime string without the +00:00 offset
*/
export const removeOffset = (datetime: string): string => {
return datetime.split('+')[0];
Expand Down
Loading
Loading