-
Notifications
You must be signed in to change notification settings - Fork 755
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
Add messaging for CLI pages command being in Beta #661
Conversation
🦋 Changeset detectedLatest commit: eb18517 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2035403003/npm-package-wrangler-661 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/661/npm-package-wrangler-661 Or you can use npx https://prerelease-registry.developers.workers.dev/runs/2035403003/npm-package-wrangler-661 dev path/to/script.js |
84745ef
to
aa4072f
Compare
aa4072f
to
4013901
Compare
packages/wrangler/src/index.tsx
Outdated
.command(subHelp) | ||
.epilog( | ||
chalk.yellow( | ||
"'wrangler pages <command>' is a Beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose" |
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.
will this message show for every pages command?
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.
Yes. It run anytime the command is utilized.
If that is too often, alternatively, it could be added in the help message.
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.
Doesn't seem to when I'm testing?
npx wrangler pages --help
'wrangler pages <command>' is a Beta command. Please report any issues to https://github.com/cloudflare/wrangler2/issues/new/choose
npx wrangler pages dev --help
No warning message.
npx wrangler pages dev <dir>
No warning message.
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.
good catch, retracting my stamp
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.
I will add more comprehensive testing around it, and fix that.
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.
unit testing subcommands was not feasible, but the tests in examples showed it working as expected on subcommand usage.
Screenshots added to PR
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, one nit.
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.
the message should show for all pages subcommands
4013901
to
2705611
Compare
2705611
to
f8391e9
Compare
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.
Nice work @JacobMGEvans - just a few requests, please.
"pages", | ||
"⚡️ Configure Cloudflare Pages", | ||
async (pagesYargs) => { | ||
await pages(pagesYargs.command(subHelp).epilogue(pagesBetaWarning)); |
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.
Ooh - epilogue!
f8391e9
to
bb9a72f
Compare
bb9a72f
to
eb18517
Compare
Looks like legitimate CI failures. |
eb18517
to
2edcba0
Compare
In #661, we wrote a test, but didn't actually test if the error gets thrown. Being explicit about the thrown error shows that the error doesn't get thrown, and the warning doesn't get logged.
* bug: `wrangler pages functions` does not log a beta warning In #661, we wrote a test, but didn't actually test if the error gets thrown. Being explicit about the thrown error shows that the error doesn't get thrown, and the warning doesn't get logged. * Added option to Pages subcommand Function to trigger error message and check for beta message Co-authored-by: Jacob M-G Evans <jacobmgevans@gmail.com>
Adding "Beta" messaging around the CLI command for Pages.
Explicitly specifying the command is Beta, not to be confused with Pages itself which is production ready, the messaging could need additional tweaking to prevent any confusion between
Pages
andnpx wrangler pages <command>
@nevikashah
resolves #642