chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues #5771

christian-bromann · 2024-05-16T23:18:43Z

What is the current behavior?

We have a lot of strict type issues in the project that need to get fixed.

What is the new behavior?

Squash some of them.

Documentation

Does this introduce a breaking change?

Yes
No

Testing

n/a

Other information

n/a

github-actions · 2024-05-16T23:20:05Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1078 errors on this branch.

That's 42 fewer than on main! 🎉🎉🎉

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/dev-server/server-process.ts	32
src/compiler/prerender/prerender-main.ts	22
src/runtime/client-hydrate.ts	20
src/runtime/vdom/vdom-render.ts	20
src/screenshot/connector-base.ts	19
src/testing/puppeteer/puppeteer-element.ts	19
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/runtime/connected-callback.ts	14
src/sys/node/node-sys.ts	14
src/compiler/prerender/prerender-queue.ts	13
src/compiler/sys/in-memory-fs.ts	13
src/runtime/set-value.ts	13
src/compiler/output-targets/output-www.ts	12
src/compiler/transformers/test/parse-vdom.spec.ts	12
src/compiler/transformers/transform-utils.ts	12
src/mock-doc/test/attribute.spec.ts	12
src/compiler/build/compiler-ctx.ts	11

Our most common errors

Typescript Error Code	Count
TS2322	350
TS2345	329
TS18048	193
TS18047	76
TS2722	27
TS2532	24
TS2531	19
TS2454	14
TS2790	11
TS2352	9
TS2769	8
TS2416	7
TS2538	4
TS2493	3
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 15 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/utils/index.ts	245	NODE_TYPES
src/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	116	Env
src/compiler/app-core/app-data.ts	118	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

github-actions · 2024-05-17T00:22:39Z

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/9120932034/artifacts/1511502530

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.18.1-dev.1715905311.37bf4e2.tgz.zip && npm install ~/Downloads/stencil-core-4.18.1-dev.1715905311.37bf4e2.tgz

alicewriteswrongs

looks good to me!

alicewriteswrongs · 2024-05-22T13:05:02Z

src/testing/jest/jest-stencil-connector.ts

+      JEST_STENCIL_FACADE = new Jest28Stencil() as JestFacade;
    } else if (version === 29) {
-      JEST_STENCIL_FACADE = new Jest29Stencil();
+      JEST_STENCIL_FACADE = new Jest29Stencil() as JestFacade;


Just curious, why were these changes necessary?

There are some type mismatches as VS Code fails to recognize multiple versions of Jest as usually us devs have only one installed. This makes the error "disappear".

Why do we have to type cast the v28 and v29 ones but not v27?

alicewriteswrongs · 2024-05-22T13:06:24Z

src/screenshot/screenshot-fs.ts

@@ -16,7 +16,7 @@ export async function writeScreenshotData(dataDir: string, screenshotData: d.Scr
 }

 export async function readScreenshotData(dataDir: string, screenshotId: string) {
-  let rtn: d.Screenshot = null;
+  let rtn: d.Screenshot | null = null;


👍 there's so many of these things scattered around the codebase, like how do you go and look at

const foo: string = null;

and think yup, looks good!

alicewriteswrongs · 2024-05-22T13:07:13Z

src/screenshot/pixel-match.ts

+    if (typeof process.send !== 'function') {
+      throw new Error('`getMismatchedPixels` must be run in a child process.');
+    }


nice, type safety and a nice helpful error message that guards against unsupported usage 👍

alicewriteswrongs · 2024-05-22T13:10:31Z

src/runtime/host-listener.ts

+  if (BUILD.hostListenerTargetParent && flags & LISTENER_FLAGS.TargetParent && elm.parentElement)
+    return elm.parentElement;


I think this should be safe, looks like the only place that getHostListenerTarget is called is right here:

stencil/src/runtime/host-listener.ts

Lines 34 to 40 in 5a82d01

listeners.map(([flags, name, method]) => {

const target = BUILD.hostListenerTarget ? getHostListenerTarget(elm, flags) : elm;

const handler = hostListenerProxy(hostRef, method);

const opts = hostListenerOpts(flags);

plt.ael(target, name, handler, opts);

(hostRef.$rmListeners$ = hostRef.$rmListeners$ || []).push(() => plt.rel(target, name, handler, opts));

});

and I think if the return was undefined before the result would have just been not adding the event listener

If target is undefined and we would call plt.ael(target, name, handler, opts), wouldn't this try to call addEventListener on undefined then?

yeah, which wouldn't throw an error, there just wouldn't be an event listener attached to any element

tanner-reits

One question (non-blocking)

tanner-reits · 2024-05-23T19:42:52Z

src/testing/jest/jest-stencil-connector.ts

+      JEST_STENCIL_FACADE = new Jest28Stencil() as JestFacade;
    } else if (version === 29) {
-      JEST_STENCIL_FACADE = new Jest29Stencil();
+      JEST_STENCIL_FACADE = new Jest29Stencil() as JestFacade;


Why do we have to type cast the v28 and v29 ones but not v27?

christian-bromann · 2024-05-24T17:54:53Z

Why do we have to type cast the v28 and v29 ones but not v27?

Because we have currently Jest v27 installed as dev dependencies which is what is being picked up by the IDE.

chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues

f2b5477

christian-bromann requested a review from a team as a code owner May 16, 2024 23:18

christian-bromann requested review from rwaskiewicz and alicewriteswrongs May 16, 2024 23:18

remove comment

f6d945f

alicewriteswrongs approved these changes May 22, 2024

View reviewed changes

christian-bromann requested a review from tanner-reits May 23, 2024 15:41

tanner-reits approved these changes May 23, 2024

View reviewed changes

christian-bromann added this pull request to the merge queue May 24, 2024

Merged via the queue into main with commit 979703b May 24, 2024
93 checks passed

christian-bromann deleted the cb/strict-fixes branch May 24, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues #5771

chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues #5771

christian-bromann commented May 16, 2024

github-actions bot commented May 16, 2024 •

edited

github-actions bot commented May 17, 2024

alicewriteswrongs left a comment

alicewriteswrongs May 22, 2024

christian-bromann May 22, 2024

tanner-reits May 23, 2024

alicewriteswrongs May 22, 2024

christian-bromann May 22, 2024

alicewriteswrongs May 22, 2024

alicewriteswrongs May 22, 2024

christian-bromann May 22, 2024

alicewriteswrongs May 22, 2024

tanner-reits left a comment

tanner-reits May 23, 2024

christian-bromann commented May 24, 2024

		if (BUILD.hostListenerTargetParent && flags & LISTENER_FLAGS.TargetParent && elm.parentElement)
		return elm.parentElement;

	listeners.map(([flags, name, method]) => {
	const target = BUILD.hostListenerTarget ? getHostListenerTarget(elm, flags) : elm;
	const handler = hostListenerProxy(hostRef, method);
	const opts = hostListenerOpts(flags);
	plt.ael(target, name, handler, opts);
	(hostRef.$rmListeners$ = hostRef.$rmListeners$ \|\| []).push(() => plt.rel(target, name, handler, opts));
	});

chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues #5771

chore(cli,client,runtime,screenshot,sys,testing): fix typescript issues #5771

Conversation

christian-bromann commented May 16, 2024

What is the current behavior?

What is the new behavior?

Documentation

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented May 16, 2024 • edited

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented May 17, 2024

PR built and packed!

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann commented May 24, 2024

github-actions bot commented May 16, 2024 •

edited

`--strictNullChecks` error report