fix(cli): fix a bug in CLI argument parsing #5646

alicewriteswrongs · 2024-04-10T15:02:38Z

This fixes a bug related to parsing of Boolean CLI arguments using the --argName=value type syntax.
We intend to support using that syntax to pass a boolean CLI flag (like --watch) but actually at present the way it is 'parsed' will result in such flags always being set to true! Not good!

This is because we were setting the value of the boolean CLI flag to Boolean('false') if you pass --argName=false, which does not result in a value of false but instead a value of true. This is changed to instead use a check that the value equals 'true' to turn the value into a boolean, and regression tests are added.

This was noticed when investigating #5640, but that bug actually has to do with how we convert CLI arguments into a Jest configuration.

What is the current behavior?

If you pass a boolean CLI flag with the =value syntax it will always be set to true.

One sort of amusing way to reproduce this is to do:

npx stencil build --watch=false

which, contrary to expectations, starts the compiler in watch mode.

What is the new behavior?

now if you do something like --watch=false we correctly parse the actual value after the equals sign for a boolean CLI flag.

Does this introduce a breaking change?

Yes
No

Testing

Here's how you can test this:

create a test project
try doing npx stencil build --watch=false and note that the compiler is started in watch mode
build, pack, and then install this branch in your test project
note how the same command now does not start the compiler in watch mode

This fixes a bug related to parsing of Boolean CLI arguments using the `--argName=value` type syntax. We intend to support using that syntax to pass a boolean CLI flag (like `--watch`) but actually at present the way it is 'parsed' will result in such flags always being set to true! Not good! This is because we were setting the value of the boolean CLI flag to `Boolean('false')` if you pass `--argName=false`, which does _not_ result in a value of `false` but instead a value of `true`. This is changed to instead use a check that the value equals `'true'` to turn the value into a boolean, and regression tests are added. This was noticed when investigating #5640, but that bug actually has to do with how we convert CLI arguments into a Jest configuration.

github-actions · 2024-04-10T15:04:18Z

`--strictNullChecks` error report

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

That's the same number of errors on main, so at least we're not creating new ones!

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/testing/puppeteer/puppeteer-element.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	17
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.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/connected-callback.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/compiler/transpile/transpile-module.ts	12
src/mock-doc/test/attribute.spec.ts	12

Our most common errors

Typescript Error Code	Count
TS2322	361
TS2345	344
TS18048	204
TS18047	82
TS2722	37
TS2532	24
TS2531	21
TS2454	14
TS2790	11
TS2352	9
TS2769	8
TS2538	8
TS2416	7
TS2493	3
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 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	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	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-04-10T15:04:34Z

PR built and packed!

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

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

unzip -d ~/Downloads ~/Downloads/stencil-core-4.15.0-dev.1712761417.c55abef.tgz.zip && npm install ~/Downloads/stencil-core-4.15.0-dev.1712761417.c55abef.tgz

This adds some manual string -> boolean type coercion to the code that we use to convert CLI flags to a Jest config (after parsing them with yargs). This fixes an issue documented in #5640 where boolean CLI flags like `--watchAll` were being passed to Jest as strings, instead of as booleans, so that it was not possible to _disable_ a flag by doing `--flagName=false` (the value of the flag would be set to the string `"false"` which is a truthy value). There was also a somewhat related issue with not properly parsing boolean flags in our CLI parser which was recently fixed in #5646. fixes #5640 STENCIL-1259

This adds some manual string -> boolean type casting to the code that we use to convert CLI flags to a Jest config (after parsing them with yargs). This fixes an issue documented in #5640 where boolean CLI flags like `--watchAll` were being passed to Jest as strings, instead of as booleans, so that it was not possible to _disable_ a flag by doing `--flagName=false` (the value of the flag would be set to the string `"false"` which is a truthy value). There was also a somewhat related issue with not properly parsing boolean flags in our CLI parser which was recently fixed in #5646. Note that to get this working it was necessary to make some module resolution-related changes in a few places since there's now a dependency between the `testing/` and `cli/` modules (`testing/` now imports and using `BOOLEAN_CLI_FLAGS`). This involved a `paths` alias in the TypeScript config, esbuild- and rollup-specific module mapping configuration / aliases, and a change in the Jest config to allow `@stencil/core/cli` to be resolved in all these contexts. fixes #5640 STENCIL-1259

…5672) This adds some manual string -> boolean type casting to the code that we use to convert CLI flags to a Jest config (after parsing them with yargs). This fixes an issue documented in #5640 where boolean CLI flags like `--watchAll` were being passed to Jest as strings, instead of as booleans, so that it was not possible to _disable_ a flag by doing `--flagName=false` (the value of the flag would be set to the string `"false"` which is a truthy value). There was also a somewhat related issue with not properly parsing boolean flags in our CLI parser which was recently fixed in #5646. Note that to get this working it was necessary to make some module resolution-related changes in a few places since there's now a dependency between the `testing/` and `cli/` modules (`testing/` now imports and using `BOOLEAN_CLI_FLAGS`). This involved a `paths` alias in the TypeScript config, esbuild- and rollup-specific module mapping configuration / aliases, and a change in the Jest config to allow `@stencil/core/cli` to be resolved in all these contexts. fixes #5640 STENCIL-1259

alicewriteswrongs marked this pull request as ready for review April 10, 2024 15:10

alicewriteswrongs requested a review from a team as a code owner April 10, 2024 15:10

alicewriteswrongs requested review from christian-bromann and tanner-reits April 10, 2024 15:10

tanner-reits approved these changes Apr 12, 2024

View reviewed changes

christian-bromann approved these changes Apr 12, 2024

View reviewed changes

alicewriteswrongs added this pull request to the merge queue Apr 12, 2024

Merged via the queue into main with commit 1fdea63 Apr 12, 2024
121 checks passed

alicewriteswrongs deleted the ap/boolean-equals-arg-parse branch April 12, 2024 16:50

alicewriteswrongs mentioned this pull request Apr 15, 2024

fix(testing): perform string -> boolean type coercion for Jest config #5672

Merged

2 tasks

jcfranco mentioned this pull request Apr 16, 2024

build(deps): update dependency @stencil/core to v4.16.0 Esri/calcite-design-system#9119

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): fix a bug in CLI argument parsing #5646

fix(cli): fix a bug in CLI argument parsing #5646

alicewriteswrongs commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

github-actions bot commented Apr 10, 2024

fix(cli): fix a bug in CLI argument parsing #5646

fix(cli): fix a bug in CLI argument parsing #5646

Conversation

alicewriteswrongs commented Apr 10, 2024

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

github-actions bot commented Apr 10, 2024

--strictNullChecks error report

reports and statistics

Unused exports report

github-actions bot commented Apr 10, 2024

PR built and packed!

`--strictNullChecks` error report