Bump to v1.0.5, update README, strict type checks #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This took a lot of little changes and additional `@type` comments here and there. But I think it actually did make the code a bit better in a couple of key places. Also updated the README to include directions on including `test-page-opener` directly in a test bundle. See mbland/tomcat-servlet-testing-example#83 and commit mbland/tomcat-servlet-testing-example@4c1bdb8. Changes include: - 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 - Updated jsconfig.json to specify: ```json "lib": [ "ES2022" ], "module": "node16", "target": "es2020", "strict": true, "exclude": [ "node_modules/**", "coverage*/**", "jsdoc/**" ] ``` The "lib", "modules", and "target" lines are to ensure compatibility with Node v18, and there's no more disabling `strictNullChecks` and `noImplicitAny` after `"strict": true`. Most of the changes in this commit are a result of removing those two options. - Added the jsdoc-plugin-intersection JSDoc plugin to prevent TypeScript intersection types from breaking `pnpm jsdoc`. I needed to use an intersection type in the `@typedef` for `CovWindow` to fix the `window` types when setting the Istanbul coverage map. Neither the JSDoc `@extends` tag or a union type (`|`) would do. - https://www.npmjs.com/package/jsdoc-plugin-intersection - https://stackoverflow.com/questions/36737921/how-to-extend-a-typedef-parameter-in-jsdoc - jsdoc/jsdoc#1285 - Defined `@typedef CovWindow` to eliminate warnings in the `BrowserPageOpener` code for creating and merging coverage maps. - Added a check for `window.open()` returning `null` in `BrowserPageOpener.open()`, along with a new test covering this case. - Defined `@typedef JSDOM` to eliminate warnings in `JsdomPageOpener`. - Restored globalThis.{document,window} instead of deleting them, and added a test to validate this. This also fixed a subtle bug whereby calling `reject(err)` previously allowed the rest of the function to continue. (Thanks for forcing me to look at this, TypeScript!) - Added @types/{chai,istanbul-lib-coverage,jsdom} to devDependencies and added a `pnpm typecheck` command. Now the whole project and its dependencies pass strict type checking. - Updated everything under test/ and test-modules/ to be strictly TypeScript compiliant. - Some "TestPageOpener > loads page with module successfully" assertions had to be extended with `|| {}`. This is because TypeScript doesn't recognize the `expect(...).not.toBeNull()` expression as a null check. - 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 `pnpm test:ci` to incorporate `pnpm jsdoc` and `pnpm typecheck`. - Other little style cleanups sprinkled here and there. --- Normally I'd prefer not to commit a change this large at once, and I'd likely ask someone else to break it up. Then again, each of these changes is so small and understandable, and they're thematically related to one another. Plus, the total change isn't that long. Hence, I've rationalized breaking my own rule in this instance.
I forgot that 'jsdoc' isn't actually a devDependency. D'oh! We'll get most of the benefits from `pnpm typecheck` anyway. `pnpm jsdoc` was mainly to check that the JSDoc comments remained compatible. I'll consider adding 'jsdoc' as a devDependency, but I'm on the fence.
Pull Request Test Coverage Report for Build 7456295219
💛 - Coveralls |
|
Ah, I thought that coverallsapp/github-action would merge I took a look at lcov-result-merger and I think I like what I see. I'll have to give it a try tomorrow. But for now, merging this PR, even with the slight reported coverage drop. |
mbland
added a commit
to mbland/jsdoc-cli-wrapper
that referenced
this pull request
Jan 9, 2024
Though a lot of changes, each one is relatively small, they're all thematically related, and all tests still pass. Note a lot of these descriptions also come from mbland/test-page-opener#23 or commit mbland/test-page-opener@01a79f6. - 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 - Updated jsconfig.json to specify: ```json "lib": [ "ES2022" ], "module": "node16", "target": "es2020", "strict": true, "exclude": [ "node_modules/**", "coverage*/**", "jsdoc/**" ] ``` The "lib", "modules", and "target" lines are to ensure compatibility with Node v18, and there's no more disabling `strictNullChecks` and `noImplicitAny` after `"strict": true`. Most of the changes in this commit are a result of removing those two options. - Added @types/{chai,node} to devDependencies and added a `pnpm typecheck` command. Now the whole project and its dependencies pass strict type checking. - Updated everything under test/ to be strictly TypeScript compliant. - Updated `pnpm test:ci` to incorporate `pnpm jsdoc` and `pnpm typecheck`. Added 'jsdoc' to devDependencies to enable this. - Added a `pnpm prepack` script to generate types and a package.json `"types"` entry. - Updated all local import paths to add '.js' or 'index.js' and added JSDoc comments everywhere reqired by `pnpm typecheck`. - Added null checks and corresponding tests everywhere reqired by `pnpm typecheck`.
mbland
added a commit
that referenced
this pull request
Jan 9, 2024
Follow-ups from #23 and commit fdf3d58. I decided to add 'jsdoc' to devDependencies and add `pnpm jsdoc` to `pnpm test:ci` after all. Also, as promised in #23, I've added lcov-result-merger to merge the coverage-{jsdom,browser}/lcov.info files generated by `pnpm test:ci`. This should get Coveralls to report 100% coverage again.
mbland
added a commit
to mbland/tomcat-servlet-testing-example
that referenced
this pull request
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: - 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
added a commit
to mbland/tomcat-servlet-testing-example
that referenced
this pull request
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: - 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
added a commit
to mbland/tomcat-servlet-testing-example
that referenced
this pull request
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: - 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
added a commit
to mbland/tomcat-servlet-testing-example
that referenced
this pull request
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: - 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
added a commit
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This took a lot of little changes and additional
@typecomments here and there. But I think it actually did make the code a bit better in a couple of key places.Also updated the README to include directions on including
test-page-openerdirectly in a test bundle. Seembland/tomcat-servlet-testing-example#83 and commit mbland/tomcat-servlet-testing-example@4c1bdb8.
Changes include:
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.Updated jsconfig.json to specify:
json "lib": [ "ES2022" ], "module": "node16", "target": "es2020", "strict": true, "exclude": [ "node_modules/**", "coverage*/**", "jsdoc/**" ]The "lib", "modules", and "target" lines are to ensure compatibility with Node v18, and there's no more disablingstrictNullChecksandnoImplicitAnyafter"strict": true. Most of the changes in this commit are a result of removing those two options.Added the jsdoc-plugin-intersection JSDoc plugin to prevent TypeScript intersection types from breaking
pnpm jsdoc. I needed to use an intersection type in the@typedefforCovWindowto fix thewindowtypes when setting the Istanbul coverage map. Neither the JSDoc@extendstag or a union type (|) would do.Defined
@typedef CovWindowto eliminate warnings in theBrowserPageOpenercode for creating and merging coverage maps.Added a check for
window.open()returningnullinBrowserPageOpener.open(), along with a new test covering this case.Defined
@typedef JSDOMto eliminate warnings inJsdomPageOpener.Restored globalThis.{document,window} instead of deleting them, and added a test to validate this. This also fixed a subtle bug whereby calling
reject(err)previously allowed the rest of the function to continue. (Thanks for forcing me to look at this, TypeScript!)Added @types/{chai,istanbul-lib-coverage,jsdom} to devDependencies and added a
pnpm typecheckcommand. Now the whole project and its dependencies pass strict type checking.Updated everything under test/ and test-modules/ to be strictly TypeScript compiliant.
Some "TestPageOpener > loads page with module successfully" assertions had to be extended with
|| {}. This is because TypeScript doesn't recognize theexpect(...).not.toBeNull()expression as a null check.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=missingimport path from missing.html with the test-modules/main.js file.So now
pnpm test:ci:jsdomwill use the V8 provider, andpnpm 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
pnpm test:cito incorporatepnpm jsdocandpnpm typecheck.Other little style cleanups sprinkled here and there.
Normally I'd prefer not to commit a change this large at once, and I'd likely ask someone else to break it up. Then again, each of these changes is so small and understandable, and they're thematically related to one another. Plus, the total change isn't that long. Hence, I've rationalized breaking my own rule in this instance.