Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Remove unneeded Node requirements #2055

Merged
merged 6 commits into from
Feb 14, 2022
Merged

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Feb 14, 2022

WHY are these changes introduced?

Fixes #2039

After adding a check to show the recommended versions for Ruby and Node in #1945 and #2032, we are now requiring Node for all the commands, even when they don't really need it.

WHAT is this pull request doing?

  • Fix the check_version method to only run it for the commands defining the recommended Node rage (recommend_default_node_range).
  • Only check the Node versions for app create rails and app create node, as it was before.

We could add it as well for other commands like app serve, but we should check the type of app to avoid doing it for PHP. Also, we should carefully decide where we want to show it and check what operations really need it, so I'll leave it for another moment.

How to test your changes?

Without Node:

  1. Uninstall Node (or temporarily remove your node executable with something like mv /usr/local/bin/node /usr/local/bin/node.bk)
  2. shopify login
  3. It should not complain about the Node version

With a wrong version:

  1. Install Node 12 (or change Node::TO to 12.0.0)
  2. shopify extension push
  3. It should not complain about the Node version

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@gonzaloriestra gonzaloriestra marked this pull request as ready for review February 14, 2022 10:33
@gonzaloriestra gonzaloriestra requested review from a team, hannachen and jesalerno84 and removed request for a team February 14, 2022 10:33
@@ -65,7 +65,7 @@ module Ruby
end

module Node
FROM = "12.0.0"
FROM = "14.5.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this to be aligned with the scripts requirements. Also, Node 12 EOL is in April, and this is just a recommendation anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, overall, is it still permitted to use Node 12 for running shopify-cli? It's not very clear if it is only a recommendation for certain parts and/or if it is mandatory to have at least 14.5 for other parts of shopify-cli? Please let us know. I'm running latest 12.22.10 and for the moment I cannot upgrade to Node 14+.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorand-horvath it should work as before for now, it's just a recommendation. But some parts, like scripts, won't probably work with Node 12.

@@ -42,7 +42,7 @@ module Messages
invalid_type: "The type %s is not supported. The only supported types are"\
" {{command:[ rails | node | php ]}}",
help: <<~HELP,
{{command:%s app create}}: Creates a ruby on rails app.
{{command:%s app create}}: Creates a new project in a subdirectory.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MeredithCastile we were showing this when you just run app create:

$ shopify app create
shopify app create: Creates a ruby on rails app.
  Usage: shopify app create [ rails | node | php ]

But it could also be a Node or PHP app... Let me know if you prefer another description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. Maybe this?

Creates a basic app
Usage: shopify app create [ rails | node | php ]

Follow up question — Is there a default? Or will the system default to rails?

Copy link
Contributor Author

@gonzaloriestra gonzaloriestra Feb 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no default, we show the help when the type of app is not provided. I'll update the message, thanks!

@gonzaloriestra gonzaloriestra merged commit 7e5ff13 into main Feb 14, 2022
@gonzaloriestra gonzaloriestra deleted the only-required-node-checks branch February 14, 2022 11:41
@gonzaloriestra gonzaloriestra mentioned this pull request Feb 14, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems February 14, 2022 12:33 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Node is required for all the commands
4 participants