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

wants to merge 7 commits into from

Conversation

henrycatalinismith
Copy link
Collaborator

Following up from the Slack discussion. This seems to have reached a point where it's going to move forward better as a pull request instead of just a branch. Curious about the time-related comments! Was tempted to just walk that commit back but it's worth keeping just cos I'm keen to learn more context about that!

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

This is cool! I have some comments below, and then a general comment which is that I think it would be cool to have this documentation and the Storybook documentation hosted together, perhaps even by the main NEXT.js server, so that it can all be available on a single server (without having to run three, or even two of them).

I wonder if that could be possible by writing a custom build/start script (that we can use on Vercel) that builds Storybook and TypeDoc into the NEXT.js /public directory and then builds and starts NEXT.js. What do you think?

Comment on lines +16 to +18
## 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.
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.

Comment on lines +64 to +67
/**
* Optionally override {@link shouldLoad shouldLoad} with a custom function.
* @returns {boolean} Whether the list should be loaded.
*/
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. 😊

Comment on lines +7 to +18
* ```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
* ```
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

@richardolsson
Copy link
Member

I did a tiny experiment with my idea and it worked fine!

image

Here's what I did. I just tried building and running it locally, but it should also work fine on Vercel (where we run our development previews) by just changing the build command to yarn docs:build && yarn build or whatever.

diff --git a/.gitignore b/.gitignore
index 4cb8d8bb6..8d2b553c0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -46,4 +46,8 @@ yarn-error.log*
 tsconfig.tsbuildinfo
 
 # typedoc
-/typedoc/
\ No newline at end of file
+/typedoc/
+
+# generated docs
+public/docs
+public/storybook
diff --git a/next.config.js b/next.config.js
index 865a00036..e391413ff 100644
--- a/next.config.js
+++ b/next.config.js
@@ -12,6 +12,16 @@ module.exports = {
   },
   async redirects() {
     return [
+      {
+        source: '/storybook',
+        destination: '/storybook/index.html',
+        permanent: true,
+      },
+      {
+        source: '/docs',
+        destination: '/docs/index.html',
+        permanent: true,
+      },
       {
         source: '/:prevPath*/calendar/events',
         destination: '/:prevPath*/calendar',
diff --git a/package.json b/package.json
index 964649193..61fe05a1d 100644
--- a/package.json
+++ b/package.json
@@ -3,6 +3,7 @@
   "version": "41.0.0",
   "private": true,
   "scripts": {
+    "docs:build": "storybook build --out public/storybook && typedoc --out public/docs",
     "analyze": "ANALYZE=true next build",
     "build": "next build",
     "start": "next start -p 80",
diff --git a/tsconfig.json b/tsconfig.json
index 4750c687f..610660b5f 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -46,6 +46,8 @@
     ".next/types/**/*.ts"
   ],
   "exclude": [
-    "node_modules"
+    "node_modules",
+    "public/storybook",
+    "public/docs"
   ]
 }

@richardolsson
Copy link
Member

I'm eager to hear your thoughts on my idea of deploying to a single app (for the dev server and preview builds on Vercel). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants