Create Block: Added prompt to continue when minimum system requirements not met #42151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +43 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
Everything looks good to me, just wondering if the messaging should be more clear that they are proceeding at their own risk? Is there a case were we could continue and then the build just wont work? I tested on node 12 and the build still worked but just not sure if there is a use-case where the scaffold works but then the build fails. |
gziolo
reviewed
Jul 6, 2022
There was a problem hiding this comment.
Everything works as expected. Great work tackling this enhancement.
In my testing, I discovered that check-node-version package has now a different API that causes an error to be thrown:
The same issue exists in the trunk and should be tackled seperately:
check.PROGRAMS is no longer being exported from the package, but I see that the same object is now exported in check-node-version/tools. I guess the simplest fix would be to import it with const tools = require( 'check-node-version/tools' ).
diff --git a/packages/create-block/lib/check-system-requirements.js b/packages/create-block/lib/check-system-requirements.js
index 99e542d6c4..02a18d684a 100644
--- a/packages/create-block/lib/check-system-requirements.js
+++ b/packages/create-block/lib/check-system-requirements.js
@@ -3,6 +3,7 @@
*/
const inquirer = require( 'inquirer' );
const checkSync = require( 'check-node-version' );
+const tools = require( 'check-node-version/tools' );
const { forEach } = require( 'lodash' );
const { promisify } = require( 'util' );
@@ -46,9 +47,7 @@ async function checkSystemRequirements( engines ) {
forEach( result.versions, ( { isSatisfied, wanted }, name ) => {
if ( ! isSatisfied ) {
log.info(
- check.PROGRAMS[ name ].getInstallInstructions(
- wanted.raw
- )
+ tools[ name ].getInstallInstructions( wanted.raw )
);
}
} );With the above I get:
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
|
@gziolo Thanks for the review! Then, as @ryanwelcher mentioned, maybe we should add a caveat.
|
|
The proposed interface interaction looks great to me. If @ryanwelcher agrees, we should have everything covered. Excellent work @t-hamano 🎉 |
ryanwelcher
approved these changes
Jul 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #40403
What?
This PR adds a prompt in
create-blockto ask whether to continue even if the system requirements are not met.Why?
However, as noted in this comment, there are some cases where system requirements are not determined correctly in certain environments.
How?
Added prompt to confirm whether to run the program as is even if system requirements are not met.
Testing Instructions
nodeversion below12ornpmversion below6.9.cd ./packages/create-blocknode index.jsno, confirm that the program exits as before.yes, make sure that the program will continue to runScreenshots or screencast
6664eb447d8020b2a10478812f9323c0.mp4