Bump to v1.0.5, update README, strict type checks

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.

Author: mbland

.eslintrc

@@ -1,7 +1,7 @@
 {
-  "env" : {
+  "env": {
     "node": true,
-    "es2023" : true
+    "es2023": true
   },
   "parserOptions": {
     "ecmaVersion": "latest",
@@ -25,7 +25,7 @@
       ]
     }
   ],
-  "rules" : {
+  "rules": {
     "@stylistic/js/comma-dangle": [
       "error", "never"
     ],
@@ -50,5 +50,12 @@
     "no-console": [
       "error", { "allow": [ "warn", "error" ]}
     ]
+  },
+  "settings": {
+    "jsdoc": {
+      "preferredTypes": {
+        "Object": "object"
+      }
+    }
   }
 }

README.md

@@ -43,6 +43,39 @@ describe('TestPageOpener', () => {
 })
 ```
 
+### Using with a bundler (e.g., with Rollup, Vite, and Vitest)
+
+If your project uses any bundler plugins that perform source transforms, you
+_may_ need to configure your project to include `test-page-loader` in the test
+bundle. Specifically, if it transforms files without a `.js` extension into importable JavaScript, `test-page-opener` may fail with an error resembling:
+
+```text
+Caused by: TypeError: Unknown file extension ".hbs" for
+/.../mbland/tomcat-servlet-testing-example/strcalc/src/main/frontend/components/calculator.hbs
+————————————————————————————————————————————————————————
+Serialized Error: { code: 'ERR_UNKNOWN_FILE_EXTENSION' }
+————————————————————————————————————————————————————————
+```
+
+For example, using [Vite][] and [Vitest][], which use [Rollup][] under the hood,
+you will need to add this `server:` setting to the `test` config object:
+
+```js
+test: {
+  server: {
+    deps: {
+      // Without this, jsdom tests will fail to import '.hbs' files
+      // transformed by rollup-plugin-handlebars-precompiler.
+      inline: ['test-page-opener']
+    }
+  }
+}
+```
+
+For a concrete example with more details, see:
+
+- <https://github.com/mbland/tomcat-servlet-testing-example/pull/83>
+
 ### Reporting code coverage
 
 `TestPageOpener` makes it possible to collect code coverage from opened browser
@@ -229,11 +262,12 @@ level explanation.
 [coveralls-tpo]: https://coveralls.io/github/mbland/test-page-opener?branch=main
 [npm-tpo]: https://www.npmjs.com/package/test-page-opener
 [pnpm]: https://pnpm.io/
+[Vite]: https://vitejs.dev/
+[Vitest]: https://vitest.dev/
+[Rollup]: https://rollupjs.org/
 [DOMContentLoaded]: https://developer.mozilla.org/docs/Web/API/Document/DOMContentLoaded_event
 [window.load]: https://developer.mozilla.org/docs/Web/API/Window/load_event
 [DOM]: https://developer.mozilla.org/docs/Web/API/Document_Object_Model
-[Vite]: https://vitejs.dev/
-[Vitest]: https://vitest.dev/
 [ECMAScript Modules]: https://nodejs.org/docs/latest-v18.x/api/esm.html
 [ESM resolution and loading algorithm]: https://nodejs.org/docs/latest-v18.x/api/esm.html#resolution-and-loading-algorithm
 [jsdom-2475]: https://github.com/jsdom/jsdom/issues/2475

ci/vitest.config.browser.js

@@ -6,6 +6,7 @@ export default mergeConfig(baseConfig, defineConfig({
   test: {
     outputFile: 'TESTS-TestSuites-browser.xml',
     coverage: {
+      provider: 'istanbul',
       reportsDirectory: 'coverage-browser'
     },
     browser: {

ci/vitest.config.js

@@ -8,7 +8,6 @@ export default mergeConfig(viteConfig, defineConfig({
     reporters: [ 'junit', 'default' ],
     coverage: {
       enabled: true,
-      provider: 'istanbul',
       reporter: [ 'text', 'lcovonly' ],
       reportsDirectory: 'coverage-jsdom'
     }

index.js

@@ -102,6 +102,7 @@ export default class TestPageOpener {
    * @param {string} pagePath - path to the HTML file relative to the basePath
    *   specified during `TestPageOpener.create()`
    * @returns {Promise<OpenedPage>} - object representing the opened page
+   * @throws {Error} if pagePath is malformed or opening page failed
    */
   async open(pagePath) {
     if (pagePath.startsWith('/')) {

jsconfig.json

@@ -1,10 +1,16 @@
 {
   "compilerOptions": {
     "checkJs": true,
-    "module": "nodenext",
-    "strict": true,
-    "strictNullChecks": false,
-    "noImplicitAny": false
+    "lib": [
+      "ES2022"
+    ],
+    "module": "node16",
+    "target": "es2020",
+    "strict": true
   },
-  "exclude": ["node_modules"]
+  "exclude": [
+    "node_modules/**",
+    "coverage*/**",
+    "jsdoc/**"
+  ]
 }

jsdoc.json

@@ -1,11 +1,12 @@
 {
   "plugins": [
-    "plugins/markdown"
+    "plugins/markdown",
+    "node_modules/jsdoc-plugin-intersection"
   ],
   "recurseDepth": 10,
   "source": {
     "includePattern": ".+\\.js$",
-    "exclude": ["node_modules"]
+    "exclude": ["node_modules", "test"]
   },
   "opts": {
     "destination": "jsdoc",

lib/browser.js

@@ -8,6 +8,11 @@
 import libCoverage from 'istanbul-lib-coverage'
 import { OpenedPage } from './types.js'
 
+/**
+ * Type for accessing the Istanbul coverage object in Window
+ * @typedef {Window & Object.<string, libCoverage.CoverageMap>} CovWindow
+ */
+
 /**
  * Returns the window and document from a browser-opened HTML file.
  *
@@ -20,6 +25,7 @@ import { OpenedPage } from './types.js'
 export default class BrowserPageOpener {
   #window
   #coverageKey
+  #coverageMap
 
   /**
    * @param {Window} window - the global (browser) window object
@@ -34,19 +40,26 @@ export default class BrowserPageOpener {
     // coverage. There's no harm in this, and it avoids a coverage gap for a
     // condition that, by definition, would never execute when collecting
     // coverage. We could use a directive to ignore that gap, but why bother.
-    window[covKey] = libCoverage.createCoverageMap(window[covKey])
+    const covWindow = /** @type {CovWindow} */ (window)
+    this.#coverageMap = libCoverage.createCoverageMap(covWindow[covKey])
+    covWindow[covKey] = this.#coverageMap
   }
 
   /**
    * Opens another page within a web browser.
    * @param {string} basePath - base path of the application under test
    * @param {string} pagePath - path to the HTML file relative to basePath
    * @returns {Promise<OpenedPage>} - object representing the opened page
+   * @throws {Error} if opening page failed
    */
   async open(basePath, pagePath) {
-    const w = this.#window.open(`${basePath}${pagePath}`)
+    const fullPath = `${basePath}${pagePath}`
+    const w = this.#window.open(fullPath)
+    if (w === null) throw new Error(`failed to open: ${fullPath}`)
+
     const close = () => {
-      this.#window[this.#coverageKey].merge(w[this.#coverageKey])
+      const testWindow = /** @type {CovWindow} */ (w)
+      this.#coverageMap.merge(testWindow[this.#coverageKey])
       w.close()
     }
 

lib/jsdom.js

@@ -7,6 +7,11 @@
 
 import { OpenedPage } from './types.js'
 
+/**
+ * @typedef {object} JSDOM - simulated jsdom.JSDOM
+ * @property {Function} fromFile - simulated JSDOM.fromFile
+ */
+
 /**
  * Returns window and document objects from a jsdom-parsed HTML file.
  *
@@ -61,7 +66,7 @@ export default class JsdomPageOpener {
   /**
    * Creates a JsdomPageOpener from a dynamically imported jsdom module
    * @param {object} jsdom - dynamically imported jsdom module
-   * @param {object} jsdom.JSDOM - JSDOM class from the jsdom module
+   * @param {JSDOM} jsdom.JSDOM - JSDOM class from the jsdom module
    */
   constructor({ JSDOM }) {
     this.#JSDOM = JSDOM
@@ -72,6 +77,7 @@ export default class JsdomPageOpener {
    * @param {string} _ - ignored
    * @param {string} pagePath - path to the HTML file to load
    * @returns {Promise<OpenedPage>} - object representing the opened page
+   * @throws {Error} if opening page failed
    */
   async open(_, pagePath) {
     const { window } = await this.#JSDOM.fromFile(
@@ -125,35 +131,40 @@ export default class JsdomPageOpener {
         // register closures over window and document, or specific document
         // elements. That would ensure they remain defined even after we remove
         // window and document from globalThis.
-        //
+        const { window: origWindow, document: origDocument } = globalThis
+
+        /** @param {Function} done - called after restoring original globals */
+        const resetGlobals = done => {
+          globalThis.document = origDocument
+          globalThis.window = origWindow
+          done()
+        }
+
         // @ts-expect-error
         globalThis.window = window
         globalThis.document = document
-        const Event = globalThis.window.Event
 
         try { await importModules(document) }
-        catch (err) { reject(err) }
+        catch (err) { return resetGlobals(() => {reject(err)}) }
 
         // Manually firing DOMContentLoaded again after loading modules
         // approximates the requirement that modules execute before
         // DOMContentLoaded. This means that the modules can register
         // DOMContentLoaded event listeners and have them fire here.
         //
         // We eventually fire the 'load' event again too for the same reason.
-        document.dispatchEvent(new Event(
-          'DOMContentLoaded', {bubbles: true, cancelable: false}
-        ))
+        const Event = globalThis.window.Event
+        document.dispatchEvent(
+          new Event('DOMContentLoaded', {bubbles: true, cancelable: false})
+        )
 
         // Register a 'load' listener that deletes the global window and
         // document variables. Because it's registered after any
         // DOMContentLoaded listeners have fired, it should execute after any
         // other 'load' listeners registered by any module code.
-        const resetGlobals = () => {
-          delete globalThis.document
-          delete globalThis.window
-          resolve()
-        }
-        window.addEventListener('load', resetGlobals, {once: true})
+        window.addEventListener(
+          'load', () => {resetGlobals(resolve)}, {once: true}
+        )
         window.dispatchEvent(
           new Event('load', {bubbles: false, cancelable: false})
         )

package.json

@@ -1,14 +1,17 @@
 {
   "name": "test-page-opener",
-  "version": "1.0.4",
+  "version": "1.0.5",
   "description": "Enables an application's tests to open its own page URLs both in the browser and in Node.js using jsdom",
   "main": "index.js",
   "types": "types/index.d.ts",
   "scripts": {
     "lint": "eslint --color --max-warnings 0 .",
     "test": "vitest",
-    "test:ci": "eslint --color --max-warnings 0 . && vitest run -c ci/vitest.config.js && vitest run -c ci/vitest.config.browser.js",
+    "test:ci": "pnpm lint && pnpm typecheck && pnpm jsdoc && pnpm test:ci:jsdom && pnpm test:ci:browser",
+    "test:ci:jsdom": "vitest run -c ci/vitest.config.js",
+    "test:ci:browser": "vitest run -c ci/vitest.config.browser.js",
     "jsdoc": "jsdoc-cli-wrapper -c jsdoc.json .",
+    "typecheck": "npx -p typescript tsc -p jsconfig.json --noEmit --pretty",
     "prepack": "npx -p typescript tsc ./index.js --allowJs --declaration --declarationMap --emitDeclarationOnly --outDir types"
   },
   "files": [
@@ -32,6 +35,9 @@
   "bugs": "https://github.com/mbland/test-page-opener/issues",
   "devDependencies": {
     "@stylistic/eslint-plugin-js": "^1.5.3",
+    "@types/chai": "^4.3.11",
+    "@types/istanbul-lib-coverage": "^2.0.6",
+    "@types/jsdom": "^21.1.6",
     "@vitest/browser": "^1.1.3",
     "@vitest/coverage-istanbul": "^1.1.3",
     "@vitest/coverage-v8": "^1.1.3",
@@ -40,6 +46,7 @@
     "eslint-plugin-jsdoc": "^46.10.1",
     "eslint-plugin-vitest": "^0.3.20",
     "jsdoc-cli-wrapper": "^1.0.4",
+    "jsdoc-plugin-intersection": "^1.0.4",
     "jsdom": "^23.1.0",
     "typescript": "^5.3.3",
     "vite": "^5.0.11",