test(compiler): remove strictNullChecks errors from validate-dist.ts #3333

alicewriteswrongs · 2022-04-14T17:01:38Z

this makes changes necessary to fix the errors from compiling w/
strictNullChecks set to true in
src/compiler/config/outputs/validate-dist.ts.

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior? What is the new behavior?

On the main branch turning on strictNullChecks produces 2229 errors. With the changes on this branch we get 2182, so we've fixed 47. Gotta start somewhere!

Does this introduce a breaking change?

Yes
No

Testing

Tests should all pass! The typechecker too.

You can also run npx tsc --strictNullChecks locally and verify that you get fewer errors than on main. You could also change that value in tsconfig.json temporarily and check that you don't get any compile errors in the file when opening it in your editor, or check the output to make sure there aren't any errors in validate-dist.ts.

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

rwaskiewicz · 2022-04-19T19:29:26Z

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

  const outputTarget = {
    ...o,
    dir: getAbsolutePath(config, o.dir || DEFAULT_DIR),
+    buildDir: o.buildDir || DEFAULT_BUILD_DIR,
+    collectionDir: o.collectionDir || DEFAULT_COLLECTION_DIR,


We probably want to preserve the original behavior here as well

Suggested change

collectionDir: o.collectionDir || DEFAULT_COLLECTION_DIR,

collectionDir: o.collectionDir !== undefined ? o.collectionDir : DEFAULT_COLLECTION_DIR,

rwaskiewicz · 2022-04-19T19:55:29Z

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

-  }
-
-  if (!isAbsolute(outputTarget.typesDir)) {
+  if (!isAbsolute(outputTarget.typesDir) && outputTarget.dir !== undefined) {


We've probably already validated the outputTarget.dir field here - I'd be a little concerned about having two checks (should they fall out of sync). Thoughts on either extracting the latter half of this logical into a variable and using that in this function like so:

const validateOutputTargetDist = (config: d.Config, o: d.OutputTargetDist): Required<d.OutputTargetDist> => { const outputDir = getAbsolutePath(config, o.dir || DEFAULT_DIR); // we need to create an object with a bunch of default values here so that // the typescript compiler can infer their types correctly const outputTarget = { ...o, dir: outputDir. buildDir: o.buildDir || DEFAULT_BUILD_DIR, // ... } // .. if (!isAbsolute(outputTarget.typesDir) && outputDir) {

or we could always just use the validated value directly and not check its undefined state:

if (!isAbsolute(outputTarget.typesDir)) {

unless you saw something else I didn't cover here? That's also a possibility 🙂

I think I added the !== undefined here before I made one of the other changes...but in any case I think it's not necessary anymore, so I'll remove it and just use outputTarget.dir (which typescript tells me is of type string not string | undefined)

rwaskiewicz · 2022-04-19T20:05:43Z

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

        typesDir: path.join(rootDir, 'my-dist', 'types'),
      },
      {
        esmDir: path.join(rootDir, 'my-dist', 'my-build', 'testing'),
        empty: false,
        isBrowserBuild: true,
        legacyLoaderFile: path.join(rootDir, 'my-dist', 'my-build', 'testing.js'),
-        polyfills: true,
+        polyfills: false,


🤔 Do you know what was causing this value to default to true before?

yes, and I meant to ask about this! It is this bit of code in validate-output-dist.ts:

// Lazy build for CDN in dist outputs.push({ type: DIST_LAZY, esmDir: lazyDir, systemDir: config.buildEs5 ? lazyDir : undefined, systemLoaderFile: config.buildEs5 ? join(lazyDir, namespace + '.js') : undefined, legacyLoaderFile: join(distOutputTarget.buildDir, namespace + '.js'), polyfills: distOutputTarget.polyfills !== undefined ? !!distOutputTarget.polyfills : true, isBrowserBuild: true, empty: distOutputTarget.empty, });

what do you think about defaulting to undefined instead, in validateOutputTargetDist ?

It's probably best we retain the original behavior (defaulting to undefined) here. I think we should do our best here to retain the current/original behavior for now to prevent breaking changes

rwaskiewicz · 2022-04-19T20:15:28Z

src/declarations/stencil-public-compiler.ts

@@ -1810,14 +1810,13 @@ export interface OutputTargetDist extends OutputTargetBase {

 export interface OutputTargetDistCollection extends OutputTargetBase {
  type: 'dist-collection';
-
+  empty: boolean;


These types are a part of the public interface to Stencil (in this part the config). We can't add fields to these interfaces without making them optional, otherwise that would result in a breaking change for users. We should make this optional like the other fields we added here:

Suggested change

empty: boolean;

empty?: boolean;

ah, that explains more why they are all optional!

making all these fields optional really doesn't jibe so well with strictNullchecks because if that's turned on every time we use one of the optional fields we need to check that the field is present first and all that. On the other PR I opened (which in light of this I'll probably re-look through, so don't feel any rush to review it just yet) I suggested we might want to introduce a less- and more-strictly-typed version of the Config type, so that in compiler internals and whatnot we could use a version which has no or fewer optional properties, and have our validateConfig function take a user-supplied config replete with optional properties and return this different, less optional type. What would you think about taking an approach like that? The type for internal use could be declared somewhere other than this public declaration file.

Anywhoo - mostly just trying to wrap my head around how this all works!

this makes changes necessary to fix the errors from compiling w/ `strictNullChecks` set to `true` in src/compiler/config/outputs/validate-dist.ts.

alicewriteswrongs · 2022-04-21T16:53:01Z

@rwaskiewicz I updated the PR inline with your comments

rwaskiewicz · 2022-04-21T18:13:50Z

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

  const outputTarget = {
    ...o,
    dir: getAbsolutePath(config, o.dir || DEFAULT_DIR),
+    buildDir: isString(o.buildDir) ? o.buildDir : DEFAULT_BUILD_DIR,
+    collectionDir: isString(o.collectionDir) ? o.collectionDir : DEFAULT_COLLECTION_DIR,


The original behavior for collectionDir checked for strict equality against undefined, can we restore that check?

Original:

if (outputTarget.collectionDir === undefined) { outputTarget.collectionDir = DEFAULT_COLLECTION_DIR; }

Suggested:

Suggested change

collectionDir: isString(o.collectionDir) ? o.collectionDir : DEFAULT_COLLECTION_DIR,

collectionDir: o.collectionDir !== undefined ? o.collectionDir : DEFAULT_COLLECTION_DIR,

yeah that makes sense, I'll make that change real quick

made that change in a22e380

rwaskiewicz

LGTM!

alicewriteswrongs requested a review from a team April 14, 2022 17:01

rwaskiewicz requested changes Apr 19, 2022

View reviewed changes

alicewriteswrongs added 3 commits April 21, 2022 12:48

test(compiler): remove strictNullChecks errors from validate-dist.ts

6960750

this makes changes necessary to fix the errors from compiling w/ `strictNullChecks` set to `true` in src/compiler/config/outputs/validate-dist.ts.

not sure about this

733864a

fiddling with defaulting to undefined instead of false

0721457

alicewriteswrongs force-pushed the ap/validate-dist-null-check branch from f1c63ab to 0721457 Compare April 21, 2022 16:48

rwaskiewicz requested changes Apr 21, 2022

View reviewed changes

check for undefined not isString on the collectionDir

a22e380

rwaskiewicz approved these changes Apr 21, 2022

View reviewed changes

rwaskiewicz merged commit 3d122b8 into main Apr 21, 2022

rwaskiewicz deleted the ap/validate-dist-null-check branch April 21, 2022 20:52

alicewriteswrongs mentioned this pull request Apr 25, 2022

test(config): remove strictNullChecks errors from compiler/config #3335

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(compiler): remove strictNullChecks errors from validate-dist.ts #3333

test(compiler): remove strictNullChecks errors from validate-dist.ts #3333

alicewriteswrongs commented Apr 14, 2022 •

edited

Loading

rwaskiewicz Apr 19, 2022

rwaskiewicz Apr 19, 2022

alicewriteswrongs Apr 20, 2022

rwaskiewicz Apr 19, 2022

alicewriteswrongs Apr 20, 2022

rwaskiewicz Apr 21, 2022

rwaskiewicz Apr 19, 2022

alicewriteswrongs Apr 20, 2022

alicewriteswrongs commented Apr 21, 2022

rwaskiewicz Apr 21, 2022

alicewriteswrongs Apr 21, 2022

alicewriteswrongs Apr 21, 2022

rwaskiewicz left a comment

	collectionDir: o.collectionDir \|\| DEFAULT_COLLECTION_DIR,
	collectionDir: o.collectionDir !== undefined ? o.collectionDir : DEFAULT_COLLECTION_DIR,

	collectionDir: isString(o.collectionDir) ? o.collectionDir : DEFAULT_COLLECTION_DIR,
	collectionDir: o.collectionDir !== undefined ? o.collectionDir : DEFAULT_COLLECTION_DIR,

test(compiler): remove strictNullChecks errors from validate-dist.ts #3333

test(compiler): remove strictNullChecks errors from validate-dist.ts #3333

Conversation

alicewriteswrongs commented Apr 14, 2022 • edited Loading

Pull request checklist

Pull request type

What is the current behavior? What is the new behavior?

Does this introduce a breaking change?

Testing

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

alicewriteswrongs commented Apr 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 14, 2022 •

edited

Loading