Skip to content

Commit

Permalink
Add strict type checking via TypeScript
Browse files Browse the repository at this point in the history
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.)
  • Loading branch information
mbland committed Jan 13, 2024
1 parent 218d2f7 commit dfddd81
Show file tree
Hide file tree
Showing 23 changed files with 758 additions and 285 deletions.
17 changes: 12 additions & 5 deletions strcalc/src/main/frontend/.eslintrc
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"env" : {
"browser" : true,
"env": {
"browser": true,
"node": true,
"es2023" : true
"es2023": true
},
"parserOptions": {
"ecmaVersion": "latest",
Expand All @@ -15,7 +15,7 @@
],
"extends": [
"eslint:recommended",
"plugin:jsdoc/recommended"
"plugin:jsdoc/recommended-typescript-flavor-error"
],
"overrides": [
{
Expand All @@ -26,7 +26,7 @@
]
}
],
"rules" : {
"rules": {
"@stylistic/js/comma-dangle": [
"error", "never"
],
Expand All @@ -51,5 +51,12 @@
"no-console": [
"error", { "allow": [ "warn", "error" ]}
]
},
"settings": {
"jsdoc": {
"preferredTypes": {
"Object": "object"
}
}
}
}
2 changes: 1 addition & 1 deletion strcalc/src/main/frontend/ci/vitest.config.browser.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { defineConfig, mergeConfig } from 'vitest/config'
import viteConfig, { buildDir } from '../vite.config'
import viteConfig, { buildDir } from '../vite.config.js'

export default mergeConfig(viteConfig, defineConfig({
test: {
Expand Down
2 changes: 1 addition & 1 deletion strcalc/src/main/frontend/ci/vitest.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { defineConfig, mergeConfig } from 'vitest/config'
import viteConfig from '../vite.config'
import viteConfig from '../vite.config.js'

export default mergeConfig(viteConfig, defineConfig({
test: {
Expand Down
3 changes: 2 additions & 1 deletion strcalc/src/main/frontend/components/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ export default class App {
* demonstrate how to design much larger applications for testability.
* @param {object} params - parameters made available to all initializers
* @param {Element} params.appElem - parent Element containing all components
* @param {object} params.calculators - calculator implementations
* @param {import('./calculators.js').StrCalcDescriptors} params.calculators -
* calculator implementations
*/
init(params) {
// In this example application, none of the components depend on one
Expand Down
17 changes: 11 additions & 6 deletions strcalc/src/main/frontend/components/app.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,23 @@
*/
import App from './app.js'
import { afterAll, afterEach, describe, expect, test } from 'vitest'
import StringCalculatorPage from '../test/page'
import StringCalculatorPage from '../test/page.js'

// @vitest-environment jsdom
describe('initial state after calling App.init()', () => {
const page = StringCalculatorPage.new()
/** @type {import('./calculators.js').StrCalcCallback} */
// eslint-disable-next-line no-unused-vars
const implStub = async _ => ({})

/** @type {import('./calculators.js').StrCalcDescriptors} */
const calculators = {
'first': { label: 'First calculator', impl: null },
'second': { label: 'Second calculator', impl: null },
'third': { label: 'Third calculator', impl: null }
'first': { label: 'First calculator', impl: implStub },
'second': { label: 'Second calculator', impl: implStub },
'third': { label: 'Third calculator', impl: implStub }
}

const page = StringCalculatorPage.new()

afterEach(() => page.clear())
afterAll(() => page.remove())

Expand All @@ -28,4 +34,3 @@ describe('initial state after calling App.init()', () => {
expect(e.href).toContain('%22Hello,_World!%22')
})
})

43 changes: 34 additions & 9 deletions strcalc/src/main/frontend/components/calculator.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,54 @@ export default class Calculator {
* Initializes the Calculator within the document.
* @param {object} params - parameters made available to all initializers
* @param {Element} params.appElem - parent Element containing all components
* @param {object} params.calculators - calculator implementations
* @param {import('./calculators.js').StrCalcDescriptors} params.calculators -
* calculator implementations
* @param {Function} [params.instantiate] - alternative template instantiation
* function for testing
* @returns {void}
*/
init({ appElem, calculators }) {
init({ appElem, calculators, instantiate = Template }) {
const calcOptions = Object.entries(calculators)
.map(([k, v]) => ({ value: k, label: v.label }))
const t = Template({ calcOptions })
const t = instantiate({ calcOptions })
const [ form, resultElem ] = t.children

appElem.appendChild(t)
document.querySelector('#numbers').focus()

/** @type {(HTMLInputElement | null)} */
const numbers = document.querySelector('#numbers')
if (numbers === null) return console.error('missing numbers input')
numbers.focus()

form.addEventListener(
'submit', e => Calculator.#submitRequest(e, resultElem, calculators)
'submit',
/** @param {Event} e - form submit event */
e => {Calculator.#submitRequest(e, resultElem, calculators)}
)
}

// https://simonplend.com/how-to-use-fetch-to-post-form-data-as-json-to-your-api/
/**
* @param {Event} event - form submit event
* @param {Element} resultElem - element into which to write the result
* @param {import('./calculators.js').StrCalcDescriptors} calculators -
* calculator implementations
* @returns {Promise<void>}
* @see https://simonplend.com/how-to-use-fetch-to-post-form-data-as-json-to-your-api/
*/
static async #submitRequest(event, resultElem, calculators) {
event.preventDefault()

const form = event.currentTarget
const form = /** @type {HTMLFormElement} */ (event.currentTarget)
const data = new FormData(form)
const selected = form.querySelector('input[name="impl"]:checked').value

/** @type {(HTMLInputElement | null)} */
const implInput = form.querySelector('input[name="impl"]:checked')
if (implInput === null) return console.error('missing implementation input')
const selected = implInput.value

/** @type {(HTMLParagraphElement | null)} */
const result = resultElem.querySelector('p')
if (result === null) return console.error('missing result element')

// None of the backends need the 'impl' parameter, and the Java backend
// will return a 500 if we send it.
Expand All @@ -43,7 +68,7 @@ export default class Calculator {
const response = await calculators[selected].impl(data)
result.textContent = `Result: ${response.result}`
} catch (err) {
result.textContent = err
result.textContent = /** @type {any} */ (err)
}
}
}
79 changes: 75 additions & 4 deletions strcalc/src/main/frontend/components/calculator.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,56 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

import Calculator from './calculator'
import Calculator from './calculator.js'
import Template from './calculator.hbs'
import { afterAll, afterEach, describe, expect, test, vi } from 'vitest'
import StringCalculatorPage from '../test/page'
import StringCalculatorPage from '../test/page.js'

// @vitest-environment jsdom
describe('Calculator', () => {
const page = StringCalculatorPage.new()

const setup = () => {
const postFormData = vi.fn()
/** @type {import('./calculators.js').StrCalcDescriptors} */
const calculators = {
'api': { label: 'API', impl: postFormData },
'browser': { label: 'Browser', impl: () => {} }
'browser': { label: 'Browser', impl: vi.fn() }
}

new Calculator().init({ appElem: page.appElem, calculators })
return { page, postFormData }
}

const setupConsoleErrorSpy = () => {
const consoleSpy = vi.spyOn(console, 'error')
.mockImplementationOnce(() => {})

return {
consoleSpy,
loggedError: () => {
const lastCall = consoleSpy.mock.lastCall
if (!lastCall) throw new Error('error not logged')
return lastCall
}
}
}

/**
* @param {string} numbersString - input to the StringCalculator
* @returns {FormData} - form data to submit to the implementation
*/
const expectedFormData = (numbersString) => {
const data = new FormData()
data.append('numbers', numbersString)
return data
}

afterEach(() => page.clear())
afterEach(() => {
vi.restoreAllMocks()
page.clear()
})

afterAll(() => page.remove())

test('renders form and result placeholder', async () => {
Expand Down Expand Up @@ -59,4 +83,51 @@ describe('Calculator', () => {
await expect(result).resolves.toBe('Error: D\'oh!')
expect(postFormData).toHaveBeenCalledWith(expectedFormData('2,2'))
})

test('logs error if missing numbers input element', async () => {
const { loggedError } = setupConsoleErrorSpy()
/** @type {import('./calculators.js').StrCalcDescriptors} */
const calculators = {}
/**
* @param {any} context - init parameters for template
* @returns {DocumentFragment} - template elements without #numbers element
*/
const BadTemplate = (context) => {
const t = Template({ context })
const [ form ] = t.children
const input = form.querySelector('#numbers')

if (input !== null) input.remove()
return t
}

new Calculator().init(
{ appElem: page.appElem, calculators, instantiate: BadTemplate }
)

expect(await vi.waitFor(loggedError))
.toStrictEqual(['missing numbers input'])
})

test('logs error if missing implementation input element', async () => {
const { page } = setup()
const { loggedError } = setupConsoleErrorSpy()

page.impl().remove()
page.enterValueAndSubmit('2,2')

expect(await vi.waitFor(loggedError))
.toStrictEqual(['missing implementation input'])
})

test('logs error if missing result element', async () => {
const { page } = setup()
const { loggedError } = setupConsoleErrorSpy()

page.result().remove()
page.enterValueAndSubmit('2,2')

expect(await vi.waitFor(loggedError))
.toStrictEqual(['missing result element'])
})
})
37 changes: 34 additions & 3 deletions strcalc/src/main/frontend/components/calculators.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,58 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/
/* global STRCALC_BACKEND */

import { postFormData } from './request.js'

export const DEFAULT_ENDPOINT = './add'

const backendUrl = () => globalThis.STRCALC_BACKEND ?
new URL(DEFAULT_ENDPOINT, globalThis.STRCALC_BACKEND).toString() :
const backendUrl = () => STRCALC_BACKEND ?
new URL(DEFAULT_ENDPOINT, STRCALC_BACKEND).toString() :
DEFAULT_ENDPOINT

const backendCalculator = async (data)=> postFormData(backendUrl(), data)
/**
* @typedef {object} StrCalcPayload
* @property {number} [result] - the result of the calculation
* @property {string} [error] - error message if the request failed
*/

/**
* Function that invokes a specific String Calculator implementation
* @callback StrCalcCallback
* @param {FormData} data - form data providing String Calculator input
* @returns {Promise<StrCalcPayload>} - the String Calculator result
*/

/**
* Posts the String Calculator input to the backend implementation
* @type {StrCalcCallback}
*/
const backendCalculator = async (data) => postFormData(backendUrl(), data)

/**
* Returns an error as a placeholder for an in-browser StringCalculator
* @type {StrCalcCallback}
*/
const tempCalculator = async (data) => Promise.reject(new Error(
`Temporary in-browser calculator received: "${data.get('numbers')}"`
))

/**
* Describes a specific StringCalculator implementation
* @typedef {object} StrCalcDescriptor
* @property {string} label - descriptive name describing the implementation
* @property {StrCalcCallback} impl - callback invoking StringCalculator impl
*/

/**
* Collection of production String Calculator implementations
*
* Each implementation takes a FormData instance containing only a
* 'numbers' field as its single argument.
* @typedef {Object.<string, StrCalcDescriptor>} StrCalcDescriptors
*/
/** @type {StrCalcDescriptors} */
export default {
'api': { label: 'Tomcat backend API (Java)', impl: backendCalculator },
'browser': { label: 'In-browser (JavaScript)', impl: tempCalculator }
Expand Down
14 changes: 9 additions & 5 deletions strcalc/src/main/frontend/components/calculators.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

import { default as calculators, DEFAULT_ENDPOINT } from './calculators'
import { default as calculators, DEFAULT_ENDPOINT } from './calculators.js'
import { afterEach, describe, expect, test, vi } from 'vitest'
import setupFetchStub from '../test/fetch-stub'
import { postOptions } from './request'
import setupFetchStub from '../test/fetch-stub.js'
import { postOptions } from './request.js'

describe('calculators', () => {
/**
* @param {string} numbersStr - input to the String Calculator
* @returns {FormData} - form data to submit to the String Calculator
*/
const setupData = (numbersStr) => {
const data = new FormData()
data.append('numbers', numbersStr)
Expand All @@ -22,7 +26,7 @@ describe('calculators', () => {
describe('defaultPost', () => {
test('posts same server by default', async () => {
const data = setupData('2,2')
const fetchStub = setupFetchStub(JSON.stringify({ result: 5 }))
const fetchStub = setupFetchStub({ result: 5 })

await expect(calculators.api.impl(data)).resolves.toEqual({ result: 5 })
expect(fetchStub).toHaveBeenCalledWith(
Expand All @@ -31,7 +35,7 @@ describe('calculators', () => {

test('posts to globalThis.STRCALC_BACKEND', async () => {
const data = setupData('2,2')
const fetchStub = setupFetchStub(JSON.stringify({ result: 5 }))
const fetchStub = setupFetchStub({ result: 5 })
vi.stubGlobal('STRCALC_BACKEND', 'http://localhost:8080/strcalc/')

await expect(calculators.api.impl(data)).resolves.toEqual({ result: 5 })
Expand Down
Loading

0 comments on commit dfddd81

Please sign in to comment.