Skip to content

Commit b5df30e

Browse files
committed
Add strict type checking via TypeScript
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.)
1 parent 218d2f7 commit b5df30e

23 files changed

+1249
-283
lines changed

strcalc/src/main/frontend/.eslintrc

+12-5
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
{
2-
"env" : {
3-
"browser" : true,
2+
"env": {
3+
"browser": true,
44
"node": true,
5-
"es2023" : true
5+
"es2023": true
66
},
77
"parserOptions": {
88
"ecmaVersion": "latest",
@@ -15,7 +15,7 @@
1515
],
1616
"extends": [
1717
"eslint:recommended",
18-
"plugin:jsdoc/recommended"
18+
"plugin:jsdoc/recommended-typescript-flavor-error"
1919
],
2020
"overrides": [
2121
{
@@ -26,7 +26,7 @@
2626
]
2727
}
2828
],
29-
"rules" : {
29+
"rules": {
3030
"@stylistic/js/comma-dangle": [
3131
"error", "never"
3232
],
@@ -51,5 +51,12 @@
5151
"no-console": [
5252
"error", { "allow": [ "warn", "error" ]}
5353
]
54+
},
55+
"settings": {
56+
"jsdoc": {
57+
"preferredTypes": {
58+
"Object": "object"
59+
}
60+
}
5461
}
5562
}

strcalc/src/main/frontend/ci/vitest.config.browser.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { defineConfig, mergeConfig } from 'vitest/config'
2-
import viteConfig, { buildDir } from '../vite.config'
2+
import viteConfig, { buildDir } from '../vite.config.js'
33

44
export default mergeConfig(viteConfig, defineConfig({
55
test: {

strcalc/src/main/frontend/ci/vitest.config.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { defineConfig, mergeConfig } from 'vitest/config'
2-
import viteConfig from '../vite.config'
2+
import viteConfig from '../vite.config.js'
33

44
export default mergeConfig(viteConfig, defineConfig({
55
test: {

strcalc/src/main/frontend/components/app.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ export default class App {
2121
* demonstrate how to design much larger applications for testability.
2222
* @param {object} params - parameters made available to all initializers
2323
* @param {Element} params.appElem - parent Element containing all components
24-
* @param {object} params.calculators - calculator implementations
24+
* @param {import('./calculators.js').StrCalcDescriptors} params.calculators -
25+
* calculator implementations
2526
*/
2627
init(params) {
2728
// In this example application, none of the components depend on one

strcalc/src/main/frontend/components/app.test.js

+11-6
Original file line numberDiff line numberDiff line change
@@ -6,17 +6,23 @@
66
*/
77
import App from './app.js'
88
import { afterAll, afterEach, describe, expect, test } from 'vitest'
9-
import StringCalculatorPage from '../test/page'
9+
import StringCalculatorPage from '../test/page.js'
1010

1111
// @vitest-environment jsdom
1212
describe('initial state after calling App.init()', () => {
13-
const page = StringCalculatorPage.new()
13+
/** @type {import('./calculators.js').StrCalcCallback} */
14+
// eslint-disable-next-line no-unused-vars
15+
const implStub = async _ => ({})
16+
17+
/** @type {import('./calculators.js').StrCalcDescriptors} */
1418
const calculators = {
15-
'first': { label: 'First calculator', impl: null },
16-
'second': { label: 'Second calculator', impl: null },
17-
'third': { label: 'Third calculator', impl: null }
19+
'first': { label: 'First calculator', impl: implStub },
20+
'second': { label: 'Second calculator', impl: implStub },
21+
'third': { label: 'Third calculator', impl: implStub }
1822
}
1923

24+
const page = StringCalculatorPage.new()
25+
2026
afterEach(() => page.clear())
2127
afterAll(() => page.remove())
2228

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

strcalc/src/main/frontend/components/calculator.js

+34-9
Original file line numberDiff line numberDiff line change
@@ -11,29 +11,54 @@ export default class Calculator {
1111
* Initializes the Calculator within the document.
1212
* @param {object} params - parameters made available to all initializers
1313
* @param {Element} params.appElem - parent Element containing all components
14-
* @param {object} params.calculators - calculator implementations
14+
* @param {import('./calculators.js').StrCalcDescriptors} params.calculators -
15+
* calculator implementations
16+
* @param {Function} [params.instantiate] - alternative template instantiation
17+
* function for testing
18+
* @returns {void}
1519
*/
16-
init({ appElem, calculators }) {
20+
init({ appElem, calculators, instantiate = Template }) {
1721
const calcOptions = Object.entries(calculators)
1822
.map(([k, v]) => ({ value: k, label: v.label }))
19-
const t = Template({ calcOptions })
23+
const t = instantiate({ calcOptions })
2024
const [ form, resultElem ] = t.children
2125

2226
appElem.appendChild(t)
23-
document.querySelector('#numbers').focus()
27+
28+
/** @type {(HTMLInputElement | null)} */
29+
const numbers = document.querySelector('#numbers')
30+
if (numbers === null) return console.error('missing numbers input')
31+
numbers.focus()
32+
2433
form.addEventListener(
25-
'submit', e => Calculator.#submitRequest(e, resultElem, calculators)
34+
'submit',
35+
/** @param {Event} e - form submit event */
36+
e => {Calculator.#submitRequest(e, resultElem, calculators)}
2637
)
2738
}
2839

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

33-
const form = event.currentTarget
51+
const form = /** @type {HTMLFormElement} */ (event.currentTarget)
3452
const data = new FormData(form)
35-
const selected = form.querySelector('input[name="impl"]:checked').value
53+
54+
/** @type {(HTMLInputElement | null)} */
55+
const implInput = form.querySelector('input[name="impl"]:checked')
56+
if (implInput === null) return console.error('missing implementation input')
57+
const selected = implInput.value
58+
59+
/** @type {(HTMLParagraphElement | null)} */
3660
const result = resultElem.querySelector('p')
61+
if (result === null) return console.error('missing result element')
3762

3863
// None of the backends need the 'impl' parameter, and the Java backend
3964
// will return a 500 if we send it.
@@ -43,7 +68,7 @@ export default class Calculator {
4368
const response = await calculators[selected].impl(data)
4469
result.textContent = `Result: ${response.result}`
4570
} catch (err) {
46-
result.textContent = err
71+
result.textContent = /** @type {any} */ (err)
4772
}
4873
}
4974
}

strcalc/src/main/frontend/components/calculator.test.js

+75-4
Original file line numberDiff line numberDiff line change
@@ -5,32 +5,56 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

8-
import Calculator from './calculator'
8+
import Calculator from './calculator.js'
9+
import Template from './calculator.hbs'
910
import { afterAll, afterEach, describe, expect, test, vi } from 'vitest'
10-
import StringCalculatorPage from '../test/page'
11+
import StringCalculatorPage from '../test/page.js'
1112

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

1617
const setup = () => {
1718
const postFormData = vi.fn()
19+
/** @type {import('./calculators.js').StrCalcDescriptors} */
1820
const calculators = {
1921
'api': { label: 'API', impl: postFormData },
20-
'browser': { label: 'Browser', impl: () => {} }
22+
'browser': { label: 'Browser', impl: vi.fn() }
2123
}
2224

2325
new Calculator().init({ appElem: page.appElem, calculators })
2426
return { page, postFormData }
2527
}
2628

29+
const setupConsoleErrorSpy = () => {
30+
const consoleSpy = vi.spyOn(console, 'error')
31+
.mockImplementationOnce(() => {})
32+
33+
return {
34+
consoleSpy,
35+
loggedError: () => {
36+
const lastCall = consoleSpy.mock.lastCall
37+
if (!lastCall) throw new Error('error not logged')
38+
return lastCall
39+
}
40+
}
41+
}
42+
43+
/**
44+
* @param {string} numbersString - input to the StringCalculator
45+
* @returns {FormData} - form data to submit to the implementation
46+
*/
2747
const expectedFormData = (numbersString) => {
2848
const data = new FormData()
2949
data.append('numbers', numbersString)
3050
return data
3151
}
3252

33-
afterEach(() => page.clear())
53+
afterEach(() => {
54+
vi.restoreAllMocks()
55+
page.clear()
56+
})
57+
3458
afterAll(() => page.remove())
3559

3660
test('renders form and result placeholder', async () => {
@@ -59,4 +83,51 @@ describe('Calculator', () => {
5983
await expect(result).resolves.toBe('Error: D\'oh!')
6084
expect(postFormData).toHaveBeenCalledWith(expectedFormData('2,2'))
6185
})
86+
87+
test('logs error if missing numbers input element', async () => {
88+
const { loggedError } = setupConsoleErrorSpy()
89+
/** @type {import('./calculators.js').StrCalcDescriptors} */
90+
const calculators = {}
91+
/**
92+
* @param {any} context - init parameters for template
93+
* @returns {DocumentFragment} - template elements without #numbers element
94+
*/
95+
const BadTemplate = (context) => {
96+
const t = Template({ context })
97+
const [ form ] = t.children
98+
const input = form.querySelector('#numbers')
99+
100+
if (input !== null) input.remove()
101+
return t
102+
}
103+
104+
new Calculator().init(
105+
{ appElem: page.appElem, calculators, instantiate: BadTemplate }
106+
)
107+
108+
expect(await vi.waitFor(loggedError))
109+
.toStrictEqual(['missing numbers input'])
110+
})
111+
112+
test('logs error if missing implementation input element', async () => {
113+
const { page } = setup()
114+
const { loggedError } = setupConsoleErrorSpy()
115+
116+
page.impl().remove()
117+
page.enterValueAndSubmit('2,2')
118+
119+
expect(await vi.waitFor(loggedError))
120+
.toStrictEqual(['missing implementation input'])
121+
})
122+
123+
test('logs error if missing result element', async () => {
124+
const { page } = setup()
125+
const { loggedError } = setupConsoleErrorSpy()
126+
127+
page.result().remove()
128+
page.enterValueAndSubmit('2,2')
129+
130+
expect(await vi.waitFor(loggedError))
131+
.toStrictEqual(['missing result element'])
132+
})
62133
})

strcalc/src/main/frontend/components/calculators.js

+34-3
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,58 @@
33
* License, v. 2.0. If a copy of the MPL was not distributed with this
44
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
55
*/
6+
/* global STRCALC_BACKEND */
67

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

910
export const DEFAULT_ENDPOINT = './add'
1011

11-
const backendUrl = () => globalThis.STRCALC_BACKEND ?
12-
new URL(DEFAULT_ENDPOINT, globalThis.STRCALC_BACKEND).toString() :
12+
const backendUrl = () => STRCALC_BACKEND ?
13+
new URL(DEFAULT_ENDPOINT, STRCALC_BACKEND).toString() :
1314
DEFAULT_ENDPOINT
1415

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

22+
/**
23+
* Function that invokes a specific String Calculator implementation
24+
* @callback StrCalcCallback
25+
* @param {FormData} data - form data providing String Calculator input
26+
* @returns {Promise<StrCalcPayload>} - the String Calculator result
27+
*/
28+
29+
/**
30+
* Posts the String Calculator input to the backend implementation
31+
* @type {StrCalcCallback}
32+
*/
33+
const backendCalculator = async (data) => postFormData(backendUrl(), data)
34+
35+
/**
36+
* Returns an error as a placeholder for an in-browser StringCalculator
37+
* @type {StrCalcCallback}
38+
*/
1739
const tempCalculator = async (data) => Promise.reject(new Error(
1840
`Temporary in-browser calculator received: "${data.get('numbers')}"`
1941
))
2042

43+
/**
44+
* Describes a specific StringCalculator implementation
45+
* @typedef {object} StrCalcDescriptor
46+
* @property {string} label - descriptive name describing the implementation
47+
* @property {StrCalcCallback} impl - callback invoking StringCalculator impl
48+
*/
49+
2150
/**
2251
* Collection of production String Calculator implementations
2352
*
2453
* Each implementation takes a FormData instance containing only a
2554
* 'numbers' field as its single argument.
55+
* @typedef {Object.<string, StrCalcDescriptor>} StrCalcDescriptors
2656
*/
57+
/** @type {StrCalcDescriptors} */
2758
export default {
2859
'api': { label: 'Tomcat backend API (Java)', impl: backendCalculator },
2960
'browser': { label: 'In-browser (JavaScript)', impl: tempCalculator }

strcalc/src/main/frontend/components/calculators.test.js

+9-5
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,16 @@
55
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
66
*/
77

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

1313
describe('calculators', () => {
14+
/**
15+
* @param {string} numbersStr - input to the String Calculator
16+
* @returns {FormData} - form data to submit to the String Calculator
17+
*/
1418
const setupData = (numbersStr) => {
1519
const data = new FormData()
1620
data.append('numbers', numbersStr)
@@ -22,7 +26,7 @@ describe('calculators', () => {
2226
describe('defaultPost', () => {
2327
test('posts same server by default', async () => {
2428
const data = setupData('2,2')
25-
const fetchStub = setupFetchStub(JSON.stringify({ result: 5 }))
29+
const fetchStub = setupFetchStub({ result: 5 })
2630

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

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

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

0 commit comments

Comments
 (0)