chore(snc): fix two snc errors related to `.typesDir` by alicewriteswrongs · Pull Request #4815 · stenciljs/core

alicewriteswrongs · 2023-09-19T18:41:17Z

Just fixing two SNC errors.

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

Just dropping some SNC errors!

Does this introduce a breaking change?

Yes
No

Testing

This shouldn't break anything since I don't think the runtime behavior of anything will really be affected. Unit tests and whatnot should all be passing.

Other information

github-actions · 2023-09-19T18:42:54Z

`--strictNullChecks` error report

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

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

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	27
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	25
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/prerender/prerender-main.ts	22
src/runtime/vdom/vdom-render.ts	20
src/runtime/client-hydrate.ts	19
src/screenshot/connector-base.ts	19
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	424
TS2322	398
TS18048	310
TS18047	100
TS2722	38
TS2532	34
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2344	5
TS2416	4
TS2493	3
TS18046	2
TS2684	1
TS2488	1
TS2430	1

Unused exports report

There are 12 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	140	CUSTOM
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	114	Env
src/compiler/app-core/app-data.ts	116	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	62	satisfies
src/compiler/types/validate-primary-package-output-target.ts	62	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

alicewriteswrongs

two notes

alicewriteswrongs · 2023-09-19T18:42:03Z

src/compiler/config/outputs/validate-dist.ts

+
+  const outputs: d.OutputTarget[] = [];
+
+  for (const outputTarget of distOutputTargets) {
+    const distOutputTarget = validateOutputTargetDist(config, outputTarget);


while I was in here I thought hey, this .reduce call is kind of weird, let's just for ... of instead

alicewriteswrongs · 2023-09-19T18:43:10Z

src/utils/output-target.ts

+ * @param outputTarget the output target of interest
+ * @returns a properly-formatted path
+ */
+export const getComponentsDtsTypesFilePath = (outputTarget: Required<d.OutputTargetDist> | d.OutputTargetDistTypes) =>


I added the Required<> wrapper here to make sure the .typesDir is defined, and this is legit since getComponentsDtsTypesFilePath is only called in the validate-dist file above where the argument satisfies that type

alicewriteswrongs

one other question

src/compiler/config/outputs/validate-dist.ts

rwaskiewicz · 2023-09-20T20:44:51Z

This code is very much in the same area as #4661 - would it be okay if we held off on this until I got to the bottom of that issue (est early next week)?

src/compiler/config/outputs/validate-dist.ts

rwaskiewicz · 2023-09-27T13:11:01Z

src/compiler/config/test/validate-output-dist.spec.ts

        isBrowserBuild: true,
        legacyLoaderFile: path.join(rootDir, 'my-dist', 'my-build', 'testing.js'),
-        polyfills: true,
+        polyfills: false,


The changes from undefined to false here make sense to me, but the value changes from true to false less so. Can you help me understand why this is happening?

This comment gives some context, basically I changed it to ensure that the value on the generated output targets matches what it set on the "dist" output target (the logic was more confusing before, if .polyfills was undefined on the dist output target then it would end up always being true on the dist-lazy one, which doesn't seem right?)

Ah - I think that comment was marked outdated and I missed it. However, I'm not sure this is the behavior we want. I could be missing something here, so just to check my understanding, this is what I think was happening and what's happening now:

In this PR, as a result of validating the dist output target invoked here, the value on the validated OutputTargetDist value could previously be set to true | false | undefined. However, now that can be true | false as of this change, and defaults to false.

Previously, we checked if polyfills was undefined. If it was, we set this value to true.

Wouldn't this change the behavior of copying over some of our polyfills? Today, I believe folks are relying on the behavior of "if I didn't set polyfills, copy them anyway" (for dist). Or am I missing something?

I agree it seems like they should be in sync

mm yeah it's a bit complicated, I guess previously we actually had three different states we treated, basically

dist.polyfills is true, then dist-lazy should be true

dist.polyfills is false, then dist-lazy should be false

dist.polyfills is undefined, then dist-lazy should be true

I think the issue is that right now the undefined case isn't distinguished from the false case, I'll make the logic here check the original output target instead of the validated one (return value of validateOutputTargetDist)

tanner-reits

Left some opinions, but LGTM

tanner-reits · 2023-10-02T18:36:28Z

src/compiler/config/test/validate-output-dist.spec.ts

        isBrowserBuild: true,
        legacyLoaderFile: path.join(rootDir, 'my-dist', 'my-build', 'testing.js'),
-        polyfills: true,
+        polyfills: false,


I agree it seems like they should be in sync

src/compiler/config/outputs/validate-dist.ts

rwaskiewicz · 2023-10-05T14:03:39Z

Ugh, sorry @alicewriteswrongs - I thought I had submitted this like, last week 😭 It's a little lost in the GH UI, but #4815 (comment) is my review/question

tanner-reits

LGTM. Lots of back-and-forth for 2 checks 😂

rwaskiewicz

One last clarifying question!

src/compiler/config/outputs/validate-dist.ts

alicewriteswrongs · 2023-10-09T18:35:06Z

It won't let me reply with a normal comment (idk how to use github apparently haha) so RE:

Is there a reason we can't use the validated DIST OT here? That way, since the return of validateOutputTargetDist is Required<OutputTargetDist> and with this change in mind where we force the return value to false if polyfills isn't a bool, this could be simplified to:

Making that change will actually change the behavior. If we use the polyfills property on the validated version then we won't distinguish between the case where polyfills is set to false on the unvalidated dist OT and when it is undefined. The existing behavior is that if it is undefined we default to true on the other OTs generated here, while if it is false then it will be false on the other OTs.

Tried to clarify that behavior here: #4815 (comment)

It's a bit weird for sure, but lmk if you think that should change or if that's not clear

This fixes two snc errors related to the `.typesDir` property and then additionally does a small refactor in `validate-dist.ts`.

alicewriteswrongs commented Sep 19, 2023

View reviewed changes

src/compiler/config/outputs/validate-dist.ts Outdated Show resolved Hide resolved

alicewriteswrongs marked this pull request as ready for review September 19, 2023 19:04

alicewriteswrongs requested a review from a team as a code owner September 19, 2023 19:04

alicewriteswrongs requested review from rwaskiewicz and tanner-reits September 19, 2023 19:04

tanner-reits requested changes Sep 20, 2023

View reviewed changes

src/compiler/config/outputs/validate-dist.ts Outdated Show resolved Hide resolved

src/compiler/config/outputs/validate-dist.ts Outdated Show resolved Hide resolved

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch from a2686d4 to d8c62ad Compare September 20, 2023 14:53

rwaskiewicz requested changes Sep 27, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch from d8c62ad to 6d3142f Compare September 27, 2023 13:38

alicewriteswrongs requested review from rwaskiewicz and tanner-reits September 28, 2023 15:37

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch 3 times, most recently from 687f7af to 8560c8c Compare October 2, 2023 14:51

tanner-reits approved these changes Oct 2, 2023

View reviewed changes

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch from 8560c8c to f068ef7 Compare October 2, 2023 18:49

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch from f068ef7 to f575d30 Compare October 6, 2023 18:41

alicewriteswrongs requested a review from tanner-reits October 6, 2023 18:42

tanner-reits approved these changes Oct 9, 2023

View reviewed changes

rwaskiewicz requested changes Oct 9, 2023

View reviewed changes

src/compiler/config/outputs/validate-dist.ts Show resolved Hide resolved

alicewriteswrongs requested a review from rwaskiewicz October 9, 2023 15:47

rwaskiewicz approved these changes Oct 9, 2023

View reviewed changes

chore(snc): fix two snc errors related to .typesDir

ad3b5cd

This fixes two snc errors related to the `.typesDir` property and then additionally does a small refactor in `validate-dist.ts`.

alicewriteswrongs force-pushed the ap/dist-types-mini-snc branch from f575d30 to ad3b5cd Compare October 9, 2023 19:15

alicewriteswrongs enabled auto-merge October 9, 2023 19:16

alicewriteswrongs added this pull request to the merge queue Oct 9, 2023

Merged via the queue into main with commit cb8d9a3 Oct 9, 2023

alicewriteswrongs deleted the ap/dist-types-mini-snc branch October 9, 2023 19:43

Uh oh!

Conversation

alicewriteswrongs commented Sep 19, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Uh oh!

github-actions bot commented Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

--strictNullChecks error report

reports and statistics

Unused exports report

Uh oh!

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwaskiewicz commented Sep 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanner-reits left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rwaskiewicz commented Oct 5, 2023

Uh oh!

tanner-reits left a comment

Choose a reason for hiding this comment

Uh oh!

rwaskiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alicewriteswrongs commented Oct 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Sep 19, 2023 •

edited

Loading

`--strictNullChecks` error report