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

Status Chart: Mega TypeScript overhaul, PR/CI checks, libcxx change #2588

Merged
merged 47 commits into from
Mar 19, 2022

Conversation

StephanTLavavej
Copy link
Member

🗺️ Overview

This upgrades the browser-rendered part of the Status Chart to TypeScript, greatly increasing its maintainability and resistance to bugs (some were found during this conversion and fixed in earlier PRs). This is increasingly important as the Status Chart has grown increasingly elaborate.

Because the browser can't directly interpret TypeScript, we need a build system. This uses tsc to perform type checking, followed by esbuild to bundle the Status Chart's main script with the daily/weekly/monthly tables. (That bundled output is checked into the gh-pages branch and updated daily. It changes at the same pace as the tables, and although it's minified, it substantially shares their content, so this should be extremely compressible and not a concern for repo bloat.) The Chart.js support machinery is still loaded through CDNs, but now via modern ECMAScript modules. (This was exceedingly difficult to figure out, but the end result is very elegant, and prevents Chart.js code from being committed to the repo in minified form. Otherwise, bundling everything would be simpler.)

We no longer have Subresource Integrity checks on the CDN scripts (because the ESMs are dynamically generated), but as this is not a mission-critical page, we'll live.

This also adds PR/CI checks to verify that gather_stats.ts and status_chart.ts will compile. These checks run directly in GitHub Actions with no Azure Pipelines involvement, and are very fast.

Now that #2499 has merged to main, this switches the libcxx line to a new methodology.

This will have a companion PR targeted at main because the daily automated updates will need new commands.

Finally, although I've put extensive effort into organizing weeks of hacking into comprehensible, isolated commits, they aren't in any particular order here (the conversion should be thought of as happening all at once, with intermediate commits not in a buildable state).

📊 src/status_chart.ts Changes

  • Rename status_chart.js to src/status_chart.ts.
  • TypeScript strict supersedes 'use strict';.
  • Import the tables.
  • Import Chart machinery.
    • We're switching from chartjs-adapter-date-fns to chartjs-adapter-luxon, as luxon is smaller when unbundled, and we've already npm installed it.
  • status_chart etc. should be separate variables.
    • Instead of being added as properties of window.
    • Fixes error TS2339: Property 'status_chart' does not exist on type 'Window & typeof globalThis'.
  • Add moreHistoryButton, lessHistoryButton.
    • Fixes error TS2531: Object is possibly 'null'.
      • (We know that they always exist.)
    • Fixes error TS2339: Property 'disabled' does not exist on type 'HTMLElement'.
      • (We know that they're always buttons.)
  • Add span as HTMLSpanElement.
    • Fixes error TS2531: Object is possibly 'null'.
      • (We know that they always exist.)
  • Add getElementByIdAs() for safety.
  • Add parameter types to legend_click_handler().
    • Fixes error TS7006: Parameter '_event' implicitly has an 'any' type. (etc.)
  • Add a Timeframe type.
    • Fixes error TS7006: Parameter 'timeframe' implicitly has an 'any' type.
  • Add parameter types to update_chart_timeframe().
    • Fixes error TS7006: Parameter 'chart' implicitly has an 'any' type. (etc.)
    • Using typeof status_chart instead of Chart avoids error TS2379: Argument of type 'Chart<X>' is not assignable to parameter of type 'Chart<Y>' [...] The types of 'config.data' are incompatible between these types.
  • Use const assertions for daily_keys, weekly_keys.
    • Fixes error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'DailyRow'. No index signature with a parameter of type 'string' was found on type 'DailyRow'. (etc.)
  • Add parameter types to get_hidden().
    • Fixes error TS7006: Parameter 'key' implicitly has an 'any' type.
  • Mark hidden as boolean.
    • Previously, TypeScript was inferring any.
  • Add HiddenInfoMaps.lookup().
    • This avoids injecting properties into the chart datasets, fixing error TS2339: Property 'stl_default_hidden' does not exist on type 'ChartDataset<...>'. (etc.)
  • Mark option strings as const.
    • Fixes errors of the form: error TS2322: Type X is not assignable to type Y. The types of 'hover.mode' are incompatible between these types. Type 'string' is not assignable to type '"index" | "nearest" | "dataset" | "x" | "y" | "point" | undefined'.
  • Mark type: 'bar' as const in merge_data.
    • Fixes error TS2322: Type X is not assignable to type Y.
  • Verify that make_xAxis() produced chart.options.scales.x.
    • Fixes error TS2532: Object is possibly 'undefined'.
    • Fixes error TS2339: Property 'time' does not exist on type X.
  • Simplify by calling make_xAxis() again.
  • Add type safety to get_values().
    • Fixes error TS7006: Parameter 'table' implicitly has an 'any' type. (etc.)
  • Replace window.onload with load_charts().
    • Previously, status_chart.js was executed immediately, so we could set window.onload and it would take effect later.
    • Now, module scripts are deferred by default. (We don't even need to wrap this in a function, but it simplifies the diff.)
  • Increase PR Age max/stepSize.

🛠️ Other Changes

  • npm install [dependencies]@latest
  • npm install chart.js chartjs-adapter-luxon esbuild http-server
  • Weekly updates and libcxx methodology change.
  • chart.js 3.7.1.
  • .vscode/settings.json: Remove terminal.integrated.cwd.
    • I was being silly - I've removed the user setting that was interfering with this.
  • .gitignore: Ignore only /built/gather_stats.js.
    • /built/status_chart.mjs will be checked in.
  • src/weekly_table.ts: Move, manually change to TypeScript.
    • We've configured TypeScript to be strict, superseding 'use strict';.
    • This exports a WeeklyRow type, where cxx17, cxx20, and lwg are optional (as we stopped recording them here when the GitHub era began).
    • Finally, this exports the weekly_table, with a type of WeeklyRow[].
  • gather_stats.ts: Simplify monthly_table printing.
  • gather_stats.ts: Simplify with multiline template literals.
    • While gather_stats.ts is a CRLF file, JavaScript/TypeScript guarantee that multiline template literals produce LF line endings, so the generated LF files won't be disrupted.
    • In write_generated_file():
      • We could use placeholders to expand the warning comment, but it's simpler to just repeat it.
      • We can now use const str.
      • To improve indentation/appearance, these template literals begin and end with a newline. We don't want the generated file to start with a newline, so we need to call str.trimStart().
      • We also need to call table_str.trim() as write_generated_file() is already emitting newlines around it.
  • gather_stats.ts: Generate TypeScript tables in src.
    • Again, we're dropping 'use strict';.
    • Some DailyRow properties can be null, when should_emit_data_point() detects that a line has fallen to 0.
  • index.html: Import modules with es-module-shims.
  • Move to src_gather/gather_stats.ts.
  • Split tsconfig.json.
    • This extracts the compilerOptions to tsconfig.universal.json.
    • Now, src_gather/tsconfig.json is for gather_stats.ts in that directory.
    • src/tsconfig.json is for status_chart.ts in that directory and adds the noEmit option, because esbuild will perform codegen after tsc performs type checking.
    • Both scripts want "moduleResolution": "node", the modern strategy.
    • For codegen, gather_stats.ts still wants "module": "CommonJS" (although it could be upgraded in the future).
    • status_chart.ts wants "module": "ES2020" (technically, this option doesn't matter because esbuild performs codegen, but I thought it would be confusing to omit).
    • This directory structure is necessary because the TypeScript ecosystem expects a tsconfig.json in the project root. (In particular, this powers VSCode squiggles, so compiler errors will be accurately diagnosed taking into account our strict settings.)
  • package.json: Add "gather", "make", "view" scripts.
  • Add .github/workflows/compile-typescript.yml.
    • Tested in my fork.
    • For gather_stats.ts, we need to pass --noEmit because we want only type checking, not codegen.
    • For status_chart.ts, it's already configured with "noEmit": true.
  • README.md: Update instructions.
    • Improve organization with a "Getting Started" header and subheaders.
    • Update Node.js version.
    • Note that gh-pages is the default, but can be customized now. (The instructions don't attempt to cover such customization.)
    • Add a section explaining how to enable GitHub Pages.
    • Extract the token-formats link for readability.
    • The instructions changed to a "public access" token in Status Chart: Weekly updates, chart.js 3.0.2. #1808, which is no longer password-equivalent, but it still should remain secret.
    • Mention that the importmap (used in the browser) should be kept in sync with the node_modules updated by npm update/npm install (and used when compiling TypeScript).
    • Update filenames.
    • npm run gather is the new way to compile and run gather_stats.ts.
    • Explain the new npm run make and npm run view scripts.
    • Add a reminder about manually updating generated files.
  • Initial built/status_chart.mjs.
    • I'll regenerate this before merging.

📈 Live Preview: stephantlavavej.github.io/STL/

Tested in Edge, Chrome, and mobile Safari (this is significant because importmaps are supported by only Chromium at this time, so es-module-shims magic is required for other browsers). I see no visual differences compared to the state before this PR, except for the new data points and PR Age rescaling.

@StephanTLavavej StephanTLavavej added documentation Related to documentation or comments infrastructure Related to repository automation labels Feb 22, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 22, 2022 09:18
@StephanTLavavej
Copy link
Member Author

Note: conflicts in daily_table.js are expected while the automated updates continue to modify gh-pages. They'll go away when I rebase before merging.

@StephanTLavavej
Copy link
Member Author

I've pushed a weekly update, dependency update, and rebased/regenerated.

@StephanTLavavej StephanTLavavej self-assigned this Mar 18, 2022
/built/status_chart.mjs will be checked in.
We've configured TypeScript to be strict, superseding `'use strict';`.

This exports a `WeeklyRow` type, where `cxx17`, `cxx20`, and `lwg` are
optional (as we stopped recording them here when the GitHub era began).

Finally, this exports the `weekly_table`, with a type of `WeeklyRow[]`.
This "join an array of strings" code was added in GH 1170, when the
daily_table used the same technique. The daily_table stopped using
join() in GH 1895. IIRC, join() allowed me to easily experiment with
reordering properties, during the early days when I avoided emitting
trailing commas. Now, join() provides no value here, just complexity.
While gather_stats.ts is a CRLF file, JavaScript/TypeScript
guarantee that multiline template literals produce LF line endings,
so the generated LF files won't be disrupted.

* In `write_generated_file()`:
    + We could use placeholders to expand the warning comment, but it's
    simpler to just repeat it.
    + We can now use `const str`.
    + To improve indentation/appearance, these template literals begin
    and end with a newline. We don't want the generated file to start
    with a newline, so we need to call `str.trimStart()`.
    + We also need to call `table_str.trim()` as `write_generated_file()`
    is already emitting newlines around it.
Again, we're dropping `'use strict';`.

Some `DailyRow` properties can be `null`, when
`should_emit_data_point()` detects that a line has fallen to 0.
We're switching from chartjs-adapter-date-fns to chartjs-adapter-luxon,
as luxon is smaller when unbundled, and we've already npm installed it.
Instead of being added as properties of `window`.

Fixes error TS2339: Property 'status_chart' does not exist on type 'Window & typeof globalThis'.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions

Fixes:
error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'DailyRow'.
  No index signature with a parameter of type 'string' was found on type 'DailyRow'.
error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'WeeklyRow'.
  No index signature with a parameter of type 'string' was found on type 'WeeklyRow'.
Fixes error TS7006: Parameter 'key' implicitly has an 'any' type.
Previously, TypeScript was inferring `any`.
This avoids injecting properties into the chart datasets, fixing:
error TS2339: Property 'stl_default_hidden' does not exist on type 'ChartDataset<...>'.
error TS2339: Property 'stl_key' does not exist on type 'ChartDataset<...>'.
Fixes errors of the form:

error TS2322: Type X is not assignable to type Y.
  The types of 'hover.mode' are incompatible between these types.
    Type 'string' is not assignable to type '"index" | "nearest" | "dataset" | "x" | "y" | "point" | undefined'.
Fixes error TS2322: Type X is not assignable to type Y.
…ns.scales.x`.

Fixes:
error TS2532: Object is possibly 'undefined'.
error TS2339: Property 'time' does not exist on type X.
Fixes:
error TS7006: Parameter 'table' implicitly has an 'any' type.
error TS7006: Parameter 'key' implicitly has an 'any' type.
error TS7006: Parameter 'row' implicitly has an 'any' type.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

Previously, status_chart.js was executed immediately, so we could set
`window.onload` and it would take effect later.

Now, module scripts are deferred by default. (We don't even need
to wrap this in a function, but it simplifies the diff.)
This extracts the compilerOptions to tsconfig.universal.json.

Now, src_gather/tsconfig.json is for gather_stats.ts in that directory.

src/tsconfig.json is for status_chart.ts in that directory
and adds the `noEmit` option, because esbuild will
perform codegen after tsc performs type checking.

Both scripts want `"moduleResolution": "node"`, the modern strategy:
https://www.typescriptlang.org/docs/handbook/module-resolution.html#module-resolution-strategies

For codegen, gather_stats.ts still wants `"module": "CommonJS"`
(although it could be upgraded in the future).

status_chart.ts wants `"module": "ES2020"` (technically,
this option doesn't matter because esbuild performs codegen,
but I thought it would be confusing to omit).
Tested in my fork.

For gather_stats.ts, we need to pass `--noEmit` because we want
only type checking, not codegen.

For status_chart.ts, it's already configured with `"noEmit": true`.
Improve organization with a "Getting Started" header and subheaders.

Update Node.js version.

Note that `gh-pages` is the default, but can be customized now.
(The instructions don't attempt to cover such customization.)

Add a section explaining how to enable GitHub Pages.

Extract the `token-formats` link for readability.

The instructions changed to a "public access" token in GH 1808, which
is no longer password-equivalent, but it still should remain secret.

Mention that the `importmap` (used in the browser) should be kept in
sync with the `node_modules` updated by `npm update`/`npm install`
(and used when compiling TypeScript).

Update filenames.

`npm run gather` is the new way to compile and run `gather_stats.ts`.

Explain the new `npm run make` and `npm run view` scripts.

Add a reminder about manually updating generated files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments infrastructure Related to repository automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants