-
Notifications
You must be signed in to change notification settings - Fork 795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cli): move version logging earlier in CLI to allow -v
, --version
#5425
Conversation
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.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/compiler/transpile/transpile-module.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/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2322 | 361 |
TS2345 | 349 |
TS18048 | 201 |
TS18047 | 82 |
TS2722 | 37 |
TS2532 | 24 |
TS2531 | 21 |
TS2454 | 14 |
TS2790 | 11 |
TS2352 | 10 |
TS2769 | 8 |
TS2538 | 8 |
TS2416 | 6 |
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 |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8147271251/artifacts/1296452637 If your browser saves files to
|
12327da
to
6580412
Compare
LGTM/works as expected! My only ask is that we update the documentation to state we support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One doc ask (should be above this one)
Docs here: ionic-team/stencil-site#1366 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated the PR summary with a link to the docs. 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This moves the version logging earlier in the `run` function for our CLI so that it's possible to log the version by doing any of - `stencil version` - `stencil -v` - `stencil --version` Prior to this change only `stencil version` would work, although we had code which was clearly intended to log the version in the case that `-v` or `--version` is passed. Background: Stencil's CLI distinguishes between _tasks_ and _flags_. A _task_ is the first CLI argument and does not start with a leading dash. It's basically treated as a subcommand, so in a CLI invocation of Stencil like `stencil build` the _task_ for Stencil will be `build`. We already support `version` as a valid task. A _flag_, by contrast, is a CLI option which controls an aspect of how Stencil runs. It's passed at the command-line like `--foo`, `--foo=bar`, or something along those lines. When there's no task passed to the stencil CLI we normally log the help text, in-line with conventional CLI UX. This means that if you do something like `stencil --build` by accident you'll get help text explaining how to invoke Stencil correctly. All well and good. However, although we've long supported `stencil version` we also have code in `main` which clearly intends to special-case the `version` _flag_ and treat it as if it is a _task_, so that if you do `stencil -v` or `stencil --version` it will be treated as if you'd done `stencil version`. The code is here: https://github.com/ionic-team/stencil/blob/1eec9a82a4d623d46228e84798adbf2f88d7f3e8/src/cli/run.ts#L56-L59 The reason that this wasn't running as intended is that earlier in the same function we would log the help text if a _task_ wasn't provided: https://github.com/ionic-team/stencil/blob/1eec9a82a4d623d46228e84798adbf2f88d7f3e8/src/cli/run.ts#L40-L44 Note the `if (!task` there. So because of this is you did `stencil -v` we would log the help text and execution wouldn't reach the lower version-related block. To fix this we just move the version logging code above the help text logging code. STENCIL-997
6580412
to
db15396
Compare
Related to ionic-team/stencil#5425 STENCIL-997
Related to ionic-team/stencil#5425 STENCIL-997
Related to ionic-team/stencil#5425 STENCIL-997
This moves the version logging earlier in the
run
function for our CLI so that it's possible to log the version by doing any ofstencil version
stencil -v
stencil --version
Prior to this change only
stencil version
would work, although we had code which was clearly intended to log the version in the case that-v
or--version
is passed.Background: Stencil's CLI distinguishes between tasks and flags.
A task is the first CLI argument and does not start with a leading dash. It's basically treated as a subcommand, so in a CLI invocation of Stencil like
stencil build
the task for Stencil will bebuild
. We already supportversion
as a valid task.A flag, by contrast, is a CLI option which controls an aspect of how Stencil runs. It's passed at the command-line like
--foo
,--foo=bar
, or something along those lines.When there's no task passed to the stencil CLI we normally log the help text, in-line with conventional CLI UX. This means that if you do something like
stencil --build
by accident you'll get help text explaining how to invoke Stencil correctly.All well and good. However, although we've long supported
stencil version
we also have code inmain
which clearly intends to special-case theversion
flag and treat it as if it is a task, so that if you dostencil -v
orstencil --version
it will be treated as if you'd donestencil version
. The code is here:stencil/src/cli/run.ts
Lines 56 to 59 in 1eec9a8
The reason that this wasn't running as intended is that earlier in the same function we would log the help text if a task wasn't provided:
stencil/src/cli/run.ts
Lines 40 to 44 in 1eec9a8
Note the
if (!task
there. So because of this is you didstencil -v
we would log the help text and execution wouldn't reach the lower version-related block.To fix this we just move the version logging code above the help text logging code.
STENCIL-997
What is the current behavior?
stencil -v
andstencil --version
don't do what you might think they do! Instead of printing the version they print the 'help' text.What is the new behavior?
They now print the version.
Documentation
ionic-team/stencil-site#1366
Does this introduce a breaking change?
Testing
Create a test project with
then
cd test-version-logging npm i
and confirm you can reproduce the issue.
Then download and install the tarball from this PR into your project, and confirm that all three variants of the 'version' command now print the version.