Warn users when they are using a Node/Ruby version that's outside of a default range #1945

pepicrft · 2022-01-19T11:34:32Z

WHY are these changes introduced?

Some users are running into issues because the version of the interpreters that they are using (e.g. Node, Ruby), is incompatible with the CLI. When that happens, users are often presented with cryptic errors that bubble up from deep pieces in the dependency graph and that makes the issues not very actionable for them.

WHAT is this pull request doing?

This PR doesn't prevent that scenario from happening because the runtime developers use is outside of our control, but prints a warning if we detect the version they are using might be incompatible with the CLI. This helps them pin-point their issue with the warning and try with a different version before opening a support ticket on our side.

How to test your changes?

Check out the branch.
Switch to a Node version that's incompatible with the CLI, for example Node 17
Run any of the commands that have the check now, for example, shopify-dev extension serve.

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.

pepicrft · 2022-01-19T11:36:07Z

lib/shopify_cli/command.rb

+        return if Environment.test?
+        return if range.nil?
+        return if version >= range.from && version < range.to
+        Context.new.warn("Your environment #{runtime} version, #{version},"\


@MeredithCastile I need your help with this content. Here's screenshot of how it looks currently:

Thanks for your patience, @pepicrft.

I'm thinking of a few things:

DISCOVERABILITY. Does this message only show up reactively when the user runs whoami? Or is there a way to surface it more proactively somehow? What's the right moment for this to show.

RECOMMENDED ACTION. As we all know, I'm not a dev. But when I ran into this issue myself, I was puzzled about what to do. What's the best way to switch between versions of node? Would we recommend a node version manager or let them figure it out on their own?

MAINTENANCE. Is there a way to automate this message when we support new versions of Node? Or will we need to remember to update the accepted range manually?

Here's a starting point to work from as we work through the above questions:

The Node version you're using (Node 17.2.0) isn't fully compatible with the Shopify CLI. Change versions by using <this NVM or doing this thing>. You can use any version from 12.0.0 to 17.0.0.

@pepicrft @amcaplan Lmk if this works. Thx!

@MeredithCastile I think we'll need to improve the error in a future release, as this has been merged already...

To answer questions as best I can:

DISCOVERABILITY. This is implemented as a hook which will run before any potentially impacted CLI action.

RECOMMENDED ACTION. Good point, I think the current version assumes developers will know how to change it but we should be more proactive about giving guidance. It's a bit complicated, as version management is hairy for Ruby (there are 3 major version managers: RVM, rbenv, chruby) and many developers aren't really Ruby developers anyway so they're just using Mac system Ruby. Needs some more thought. Node is more simple as probably most users are using nvm, but there are also n, fnm, and probably many smaller competitors because JS gonna JS. Perhaps we provide an example for each of the most popular version managers, or link to a docs page?

MAINTENANCE. The FROM is a minimum version which might be updated at some point but we'll know to do that, I think. The TO version, following some discussion today, will be more broadly scoped, so rather than keeping it pointing to the latest patch version, it'll be pointing to the next minor version we don't support (for Ruby) which is an annual event. For Node, I assume we'll adopt a similar approach, specifying the first release we don't expect to support. So when we support Node 17 fully, we'll bump it to 18 that we don't support.

lib/project_types/theme/commands/init.rb

pepicrft · 2022-01-19T11:38:15Z

lib/project_types/extension/commands/create.rb

@@ -5,6 +5,9 @@ class Command
    class Create < ShopifyCLI::Command::SubCommand
      prerequisite_task :ensure_authenticated

+      recommend_default_node_range


@t6d / @vividviolet what Node versions do we expect people to have to be able to develop extensions? Do we have tests on our end that verify we support the range?

@vividviolet, do you recall? If not, I can do some testing. I also remember running into NPM issues so we might have to restrict those versions as well.

@t6d We are using import/export in our build tools which requires Node 12 but the code should be compiled down to require statements anyway. The project itself should not require any features from node that I'm aware of.

As far as NPM issues, I think this went away as we're no longer using npm list to check for the renderer version? Can you double check this? If we removed that code then we don't really have a requirement for a specific node version.

I would still suggest a minimum version of Node 10 since we need to use fs. I know we're switching to STDIN but it would cover our bases in case we want to support reading from a config file when building the extension.

As far as NPM issues, I think this went away as we're no longer using npm list to check for the renderer version?

I will double check. I believe this is true for the new code path that uses the new development server. There still seem to be active code paths in the old implementation that utilize npm list to determine the version of the Argo CLI package.

Thanks @vividviolet & @t6d 🙏🏼. I'll wait then until I have that information to make sure it aligns with extensions' requirements.

Given that we're working towards deprecating the code path that still runs npm|yarn list, I don't think we should bother introducing a check for yarn|npm itself. Feel free to move forward with the PR as is.

lib/project_types/script/commands/connect.rb

macournoyer · 2022-01-19T13:25:31Z

For the Ruby version, how will this work w/ the required_ruby_version in our gemspec file? AFAIK, this line currently makes it impossible to install the CLI under ruby < 2.6.

Should we remove the gemspec line so our CLI can provide a clearer message? Or instead bump required_ruby_version to 2.7, and remove the custom check in our code.

pepicrft · 2022-01-20T09:55:12Z

For the Ruby version, how will this work w/ the required_ruby_version in our gemspec file? AFAIK, this line currently makes it impossible to install the CLI under ruby < 2.6.

Note that in the case of a Homebrew installation, the Ruby version used to run the CLI might not be the one that's activated in your environment. That might lead to, Shopify CLI running with a Homebrew Ruby 2.7, but trying to create a Rails embedded app with Ruby 3.1 which is incompatible with our template. Hence the existence of a second check.

Should we remove the gemspec line so our CLI can provide a clearer message? Or instead bump required_ruby_version to 2.7, and remove the custom check in our code.

I was thinking about changing 2.7 for 2.6 because it's the version that comes installed by default on macOS. What do you think?

karreiro

Thank you for this PR and for clarifying the gemspec question, @pepicrft!

I believe it would be nice to keep the Ruby range (lib/shopify_cli/constants.rb) synced with Shopify CLI CI (which currently runs on 3.0.2, 2.7.5, and 2.6.6). It's the constraint I keep in mind from the Theme CLI perspective.

Considering our CI, I don't believe changing the gemspec from 2.7 to 2.6 would be an issue. Still, we would need to include a post-release step in the PR description to update the docs side as well: https://shopify.dev/apps/tools/cli/installation.

amcaplan · 2022-01-20T15:43:19Z

Note that in the case of a Homebrew installation, the Ruby version used to run the CLI might not be the one that's activated in your environment. That might lead to, Shopify CLI running with a Homebrew Ruby 2.7, but trying to create a Rails embedded app with Ruby 3.1 which is incompatible with our template. Hence the existence of a second check.

If installed via Homebrew, the CLI will use Homebrew Ruby. It will also pin the .ruby-version in the app directory to that version of Ruby.

Note that installs/updates of the CLI via Homebrew should force an update to Ruby 3.

I was thinking about changing 2.7 for 2.6 because it's the version that comes installed by default on macOS. What do you think?

I'd advise against this. System Ruby on Mac OSX is incompatible with recent Rails versions due to OpenSSL problems, so shopify app rails create fails in the middle when doing some standard rails app init work involving secrets. This is why I've pinned the Homebrew recipe to Ruby 3. Currently the Homebrew Ruby formula is on 3.0.3.

jacobsteves · 2022-01-20T15:47:51Z

lib/shopify_cli/command.rb

+          check_ruby_version
+          check_node_version


I believe you'll also have to move these lines into the subcommand class since it overrides call

Good catch. I'll add them there too.

macournoyer · 2022-01-20T16:18:16Z

I was thinking about changing 2.7 for 2.6 because it's the version that comes installed by default on macOS. What do you think?

All good to lower to 2.6 from CLI & https://github.com/Shopify/theme-check/blob/main/theme-check.gemspec#L16 too. But haven't checked other dependencies.

andrewhassan · 2022-01-28T18:54:53Z

lib/project_types/script/commands/connect.rb

+      recommend_node(
+        from: ::Script::Layers::Infrastructure::Languages::TypeScriptProjectCreator::MIN_NODE_VERSION,
+        to: ShopifyCLI::Constants::SupportedVersions::Node::TO
+      )


@jacobsteves @lopert Please correct me if I'm wrong, but I don't believe we want to check node when we connect an app, since this command doesn't rely on node and the user could be using a language that doesn't require node (e.g. Rust, Wasm).

andrewhassan · 2022-01-28T18:57:00Z

lib/project_types/script/commands/create.rb

+      recommend_node(
+        from: ::Script::Layers::Infrastructure::Languages::TypeScriptProjectCreator::MIN_NODE_VERSION,
+        to: ShopifyCLI::Constants::SupportedVersions::Node::TO
+      )


Similar to my comment above, I believe we only require node if the user's script project requires it (e.g. a TypeScript project). I think this comment also applies to push below. cc @jacobsteves @lopert

Agreed. Could we trigger this based on the language the user selects in the menu?

I think in our case, checking from the Project Creator and TaskRunners makes more sense. Sure we could add if language != "wasm" here but it doesn't feel correct. I'd recommend not checking here.

gonzaloriestra · 2022-02-04T12:51:21Z

lib/shopify_cli/constants.rb

+      module Ruby
+        FROM = "2.6.6"
+        TO = "3.0.2"
+      end
+
+      module Node
+        FROM = "12.0.0"
+        TO = "17.0.0"
+      end


I think we should mention this in the documentation. For example, we say Ruby 2.7 or higher as a requirement, but if they choose the latest version (3.1), then the CLI will show a warning. What do you think @shainaraskas?

are you suggesting that we refer to a latest compatible version as well?
how frequently / how long do you suspect we'll be behind? there is a high risk of this falling out of date quickly.

could we consider instead saying that "it might be a couple of [weeks] before we add support for the very latest ruby" instead?

I don't think we have a node version indicated in the docs. can add if desired

Well, Ruby 3.1 was released around 6 weeks ago already 😅 , but I see your point. Another option could be to add a comment in this very piece of code to remember updating the docs. As you prefer!

how often does a new version of ruby get released? how often do we bump the version here? if the answer is always 6wk or greater, it might be ok to put a fixed value in the docs.

Ruby releases a new version every year and Node between 6-12 months.

amcaplan · 2022-02-06T10:08:20Z

lib/shopify_cli/constants.rb

+    module SupportedVersions
+      module Ruby
+        FROM = "2.6.6"
+        TO = "3.0.2"


Sorry for joining the party late, but I just saw the version of Ruby we're recommending.

Homebrew users currently are on 3.0.3 so they'll all be warned.

Can the check be a bit less strict, maybe we say < 3.1 instead of <= 3.0.2?

Good point! Looking at the code, it seems that the FROM version (2.6.6) is accepted, but the TO (3.0.2) is not...

shopify-cli/lib/shopify_cli/command.rb

Line 113 in 3284093

return if version >= range.from && version < range.to

So I think the best way would be to change that to <= and use "3.0" as the TO instead.

Great points @amcaplan @gonzaloriestra!

Supporting this comment, I believe that it would be valuable to add Ruby 3.1 into our CI as well (if we change the Ruby range).

Thus, we guarantee the CLI is stable in the suggested versions :-)

It was finally fixed here: #2022

pepicrft requested a review from a team January 19, 2022 11:34

pepicrft requested a review from a team as a code owner January 19, 2022 11:34

pepicrft requested review from jesalerno84 and amcaplan and removed request for a team January 19, 2022 11:34

pepicrft commented Jan 19, 2022

View reviewed changes

lib/project_types/theme/commands/init.rb Show resolved Hide resolved

pepicrft commented Jan 19, 2022

View reviewed changes

lib/project_types/script/commands/connect.rb Outdated Show resolved Hide resolved

pepicrft requested review from karreiro, vividviolet, jacobsteves and t6d January 19, 2022 11:40

pepicrft self-assigned this Jan 19, 2022

karreiro reviewed Jan 20, 2022

View reviewed changes

jacobsteves reviewed Jan 20, 2022

View reviewed changes

pepicrft force-pushed the version-checks branch from 38494f5 to 1a874df Compare January 26, 2022 10:10

andrewhassan reviewed Jan 28, 2022

View reviewed changes

pepicrft added 6 commits February 3, 2022 10:42

Add methods to get the Ruby and Node versions

df157e9

Pring a warning if the environment version is out of a default range

b9ce924

Align versions with CI

fd5e589

Align versions with CI and add the checks to sub_command too

6e27790

Adjust the scripts' checked version to align with their requirements

fa0286f

Remove unnecessary checks and update the CHANGELOG

3284093

pepicrft force-pushed the version-checks branch from 1a874df to 3284093 Compare February 3, 2022 09:45

pepicrft merged commit cfa705a into main Feb 3, 2022

pepicrft deleted the version-checks branch February 3, 2022 11:30

gonzaloriestra reviewed Feb 4, 2022

View reviewed changes

amcaplan reviewed Feb 6, 2022

View reviewed changes

This was referenced Feb 7, 2022

Release 2.11.0 #2021

Merged

Drop the pre and build information before doing the comparison #2022

Merged

shopify-shipit bot temporarily deployed to rubygems February 7, 2022 17:18 Inactive

amcaplan mentioned this pull request Feb 8, 2022

shopify theme pull fails with An unexpected error occured #1984

Closed

gonzaloriestra mentioned this pull request Feb 14, 2022

Remove unneeded Node requirements #2055

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn users when they are using a Node/Ruby version that's outside of a default range #1945

Warn users when they are using a Node/Ruby version that's outside of a default range #1945

pepicrft commented Jan 19, 2022

pepicrft Jan 19, 2022

MeredithCastile Jan 28, 2022

MeredithCastile Feb 7, 2022

amcaplan Feb 7, 2022

pepicrft Jan 19, 2022

t6d Jan 20, 2022

vividviolet Jan 20, 2022 •

edited

Loading

t6d Jan 20, 2022

pepicrft Jan 26, 2022 •

edited

Loading

t6d Jan 31, 2022

macournoyer commented Jan 19, 2022

pepicrft commented Jan 20, 2022

karreiro left a comment •

edited

Loading

amcaplan commented Jan 20, 2022

jacobsteves Jan 20, 2022

pepicrft Jan 26, 2022

macournoyer commented Jan 20, 2022

andrewhassan Jan 28, 2022

andrewhassan Jan 28, 2022

lopert Jan 28, 2022

lopert Feb 1, 2022 •

edited

Loading

gonzaloriestra Feb 4, 2022

shainaraskas Feb 4, 2022

gonzaloriestra Feb 7, 2022

shainaraskas Feb 8, 2022

gonzaloriestra Feb 8, 2022

amcaplan Feb 6, 2022

gonzaloriestra Feb 7, 2022

karreiro Feb 7, 2022

gonzaloriestra Feb 8, 2022

Warn users when they are using a Node/Ruby version that's outside of a default range #1945

Warn users when they are using a Node/Ruby version that's outside of a default range #1945

Conversation

pepicrft commented Jan 19, 2022

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Update checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vividviolet Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macournoyer commented Jan 19, 2022

pepicrft commented Jan 20, 2022

karreiro left a comment • edited Loading

Choose a reason for hiding this comment

amcaplan commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macournoyer commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lopert Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vividviolet Jan 20, 2022 •

edited

Loading

pepicrft Jan 26, 2022 •

edited

Loading

karreiro left a comment •

edited

Loading

lopert Feb 1, 2022 •

edited

Loading