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

Add strict type checking via TypeScript #85

Merged
merged 1 commit into from
Jan 13, 2024
Merged

Add strict type checking via TypeScript #85

merged 1 commit into from
Jan 13, 2024

Conversation

mbland
Copy link
Owner

@mbland mbland commented Jan 13, 2024

Adds Typescript to devDependencies and a jsconfig.json files to ensure Visual Studio Code performs more thorough type checking.

Description largely copied from:

The code is still JavaScript, but now we get strict type checking in Visual Studio Code and in continuous integration via tsc in pnpm typecheck.

The docs generated by 'jsdoc' are a little funky, and we don't get as much documentation in Visual Studio Code as I expected. I believe I can fix these issues at some point with this foundation in place.

The actual changes include:

Of special note:

  • Added the instantiate() parameter to Calculator.init() to inject a Handlebars Template() function. This enabled testing that a missing #numbers element was logged by Calculator.init().

    Tests for this and Calculator.#submitRequest() set up a console.error spy along with a callback for Vitest's vi.waitFor().

    I need to write a document and/or blog post about this as part of the Handlebars Component Pattern. (I just came up with that name while writing it.)

@mbland mbland self-assigned this Jan 13, 2024
@mbland mbland force-pushed the strict-typing branch 2 times, most recently from 60c61d9 to dfddd81 Compare January 13, 2024 17:37

Verified

This commit was signed with the committer’s verified signature.
AgustinBettati Agustin Bettati
Adds Typescript to devDependencies and a jsconfig.json files to ensure
Visual Studio Code performs more thorough type checking.

Description largely copied from:

- mbland/test-page-opener#22
  mbland/test-page-opener@a63f274
- mbland/test-page-opener#23
  mbland/test-page-opener@01a79f6
- mbland/jsdoc-cli-wrapper#20
  mbland/jsdoc-cli-wrapper@fafcd21
- mbland/rollup-plugin-handlebars-precompiler#7
  mbland/rollup-plugin-handlebars-precompiler@eb5b9a8
- mbland/rollup-plugin-handlebars-precompiler#8
  mbland/rollup-plugin-handlebars-precompiler@8b36b2a

The code is still JavaScript, but now we get strict type checking in
Visual Studio Code and in continuous integration via `tsc` in `pnpm
typecheck`.

The docs generated by 'jsdoc' are a little funky, and we don't get as
much documentation in Visual Studio Code as I expected. I believe I can
fix these issues at some point with this foundation in place.

The actual changes include:

- Added @types/chai, jsdoc, and typescript as devDependencies.

- Set .eslintrc to disable the no-undefined-types rule by extending
  "plugin:jsdoc/recommended-typescript-flavor-error". This is because
  the Handlebars types in lib/parser.js weren't trivial to replicate,
  and TypeScript finds those types just fine. This was based on advice
  from:

  > ...the config plugin:jsdoc/recommended-typescript-error should
  > disable the jsdoc/no-undefined-types rule because TypeScript itself
  > is responsible for reporting errors about invalid JSDoc types.
  >
  > - gajus/eslint-plugin-jsdoc#888 (comment)

  And:

  > If you are not using TypeScript syntax (your source files are still
  > .js files) but you are using the TypeScript flavor within JSDoc
  > (i.e., the default "typescript" mode in eslint-plugin-jsdoc) and you
  > are perhaps using allowJs and checkJs options of TypeScript's
  > tsconfig.json), you may use:
  >
  > ```json
  > {
  >   "extends": ["plugin:jsdoc/recommended-typescript-flavor"]
  > }
  > ```
  >
  > ...or to report with failing errors instead of mere warnings:
  >
  > ```json

  More background:

  - https://github.com/gajus/eslint-plugin-jsdoc/blob/main/docs/rules/no-undefined-types.md
  - gajus/eslint-plugin-jsdoc#99
  - gajus/eslint-plugin-jsdoc#1098
  - jsdoc/jsdoc#1537

- Added `settings.jsdoc.preferredTypes.Object = "object"` to .eslintrc
  to enable "Object.<..., ...>" syntax in a JSDoc `@typedef`. Got rid of
  some extra whitespaces in .eslintrc, too.
  - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/rules/check-types.md#why-not-capital-case-everything
  - https://github.com/gajus/eslint-plugin-jsdoc/blob/b60cbb027b03b4f6d509933b0dca8681dbe47206/docs/settings.md#settings-to-configure-check-types-and-no-undefined-types

- Added '.js' extension to all internal imports and added JSDoc comments
  everywhere reqired by `pnpm typecheck`.

- Added 'jsdoc-plugin-typescript' to the build to handle the TypeScript
  `import().Type` directives. This ended up pulling in the 'es-abstract'
  module, which blew up the pnpm-lock.yaml file. If I get an itch, I'll
  implement my own plugin one day and replace it.

- Updated `pnpm test-ci` to incorporate `pnpm jsdoc` and `pnpm
  typecheck`. Added 'jsdoc' to devDependencies to enable this.

- Added `null` checks everywhere reqired by `pnpm typecheck`. Added
  tests to cover all the `null` cases.

- Added globals.d.ts and a `/* global STRCALC_BACKEND */` ESLint comment
  to calculators.js to properly type check `globalThis.STRCALC_BACKEND`.
  Ironically, this required just referencing it as `STRCALC_BACKEND`
  without `globalThis`.

- Added a temporary components/template.d.ts containing the Handlebars
  Template() type declaration. Once I properly export those types from
  rollup-plugin-handlebars-precompiler, I'll remove it. (That plugin
  currently contains lib/template.d.ts, not types/template.d.ts.)

- Added a test for the `#missing app element` case in main.js by adding
  a new test/main-missing-app-div.test.js. I need to port it to
  mbland/test-page-opener to solve the coverage problem encountered in:

  - mbland/test-page-opener#23
    mbland/test-page-opener@01a79f6

  > - Added a new missing.html and "JsdomPageOpener > doesn't throw if
  >   missing app div" test case to cover new null check in
  >   test-modules/main.js.
  >
  >   This did, however, throw off Istanbul coverage, but not V8
  >   coverage. Running just the "doesn't throw" test case shows 0%
  >   coverage of main.js, even though the test clearly passes. My
  >   suspicion is that Istanbul can't associate the
  >   `./main.js?version=missing` import path from missing.html with the
  >   test-modules/main.js file.
  >
  >   So now `pnpm test:ci:jsdom` will use the V8 provider, and `pnpm
  >   test:ci:browser`, which doesn't use missing.html, will continue to
  >   use Istanbul. Each task outputs its own separate .lcov file which
  >   then gets merged into Coveralls.

- Updated `setupFetchStub()` to detect the type of the `body` argument
  and call `JSON.stringify()` itself if it's an `object`. This
  eliminated the need for most callers to call `JSON.stringify()`.

- Updated `StringCalculatorPage` with typing information and made it so
  that an empty object will stand in for `null` elements. This is
  playing loose with typing a bit, as any `null`s will cause errors
  showing unknown property access. But that seemed better than
  burdening all callers to do their own `null` checks or workarounds.

Of special note:

- Added the `instantiate()` parameter to Calculator.init() to inject a
  Handlebars Template() function. This enabled testing that a missing
  `#numbers` element was logged by Calculator.init().

  Tests for this and Calculator.#submitRequest() set up a console.error
  spy along with a callback for Vitest's vi.waitFor().

  I need to write a document and/or blog post about this as part of the
  Handlebars Component Pattern. (I just came up with that name while
  writing it.)
@mbland mbland merged commit 8cee0fb into main Jan 13, 2024
3 checks passed
@mbland mbland deleted the strict-typing branch January 13, 2024 17:41
mbland added a commit to mbland/test-page-opener that referenced this pull request Jan 16, 2024
This commit solves the coverage drop from:

- #23
  01a79f6

Restores `istanbul` as the coverage reporter for the JSDom-based CI run
to restore consistency in reporting, particularly in Coveralls.

---

Adapted from the new test/missing-app-div/jsdom.test.js:

Moved the "missing #app div" test into a separate directory from the
other tests in ../jsdom.test.js. to prevent Node.js from using the same
`test-modules/main.js` import. Otherwise:

- If the test case were in the same file, the "missing #app div" branch
  wouldn't execute and the test would fail.

- If this test file were in the same directory, the Istanbul coverage
  reporter wouldn't see the coverage from the "missing #app div" branch.
  I don't know exactly why that is.

At the same time, the previous `src="./main.js?version=missing"` query
suffix is no longer necessary.

I got the idea to organize the tests this way after successfully
covering similar code in:

- mbland/tomcat-servlet-testing-example#85
  mbland/tomcat-servlet-testing-example@b5df30e
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.

1 participant