Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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.)
mbland committed Jan 13, 2024

Verified

This commit was signed with the committer’s verified signature.
AgustinBettati Agustin Bettati
1 parent 218d2f7 commit dfddd81
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",
@@ -15,7 +15,7 @@
],
"extends": [
"eslint:recommended",
"plugin:jsdoc/recommended"
"plugin:jsdoc/recommended-typescript-flavor-error"
],
"overrides": [
{
@@ -26,7 +26,7 @@
]
}
],
"rules" : {
"rules": {
"@stylistic/js/comma-dangle": [
"error", "never"
],
@@ -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: {
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: {
3 changes: 2 additions & 1 deletion strcalc/src/main/frontend/components/app.js
Original file line number Diff line number Diff line change
@@ -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
17 changes: 11 additions & 6 deletions strcalc/src/main/frontend/components/app.test.js
Original file line number Diff line number Diff line change
@@ -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())

@@ -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
@@ -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.
@@ -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
@@ -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 () => {
@@ -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
@@ -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 }
14 changes: 9 additions & 5 deletions strcalc/src/main/frontend/components/calculators.test.js
Original file line number Diff line number Diff line change
@@ -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)
@@ -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(
@@ -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 })
Loading

0 comments on commit dfddd81

Please sign in to comment.