[PDE-3436] refactor(cli): Explicitly require zapier-core in commands

[robwa10]

This change requires all cli commands to declare whether they need the zapier-platform-core package.

[robwa10]

Initially I tried declaring it in a constructor.

class AnalyticsCommand extends BaseCommand {
  constructor() {
    super();
    this.requiresCore = true;
  }
// ...

But I kept getting this error, which seemed to be something with oclif.

TypeError: Cannot read properties of undefined (reading 'scopedEnvVarKey')
    at BuildCommand._run (~/z/zapier-repos/zapier-platform/packages/cli/node_modules/@oclif/command/lib/command.js:41:44)
    at Command.run (~/z/zapier-repos/zapier-platform/packages/cli/node_modules/@oclif/command/lib/command.js:162:16)
    at async Config.runCommand (~/z/zapier-repos/zapier-platform/packages/cli/node_modules/@oclif/config/lib/config.js:173:24)
    at async Main.run (~/z/zapier-repos/zapier-platform/packages/cli/node_modules/@oclif/command/lib/main.js:27:9)
    at async Main._run (~/z/zapier-repos/zapier-platform/packages/cli/node_modules/@oclif/command/lib/command.js:43:20)  

If you've got any insight happy to hear what might be causing it. I didn't see anything in Oclif's docs and couldn't find anything relevant on Google.

[robwa10]

I think I set the requirement too true for all the packages that do require core but I'm no where near 100% sure on that. And I don't have a huge amount of faith that the unit tests would actually catch it if it's wrong for a command.

[robwa10]

Tests failed without this here.

[robwa10]

Same as above.

[eliangcs]

@robwa10 I just found we already have a flag called skipValidInstallCheck to disable the core installation check. Some commands are already using it:

$ rg 'skipValidInstallCheck'
packages/cli/src/oclif/ZapierBaseCommand.js
75:    if (this._staticClassReference.skipValidInstallCheck) {
286:ZapierBaseCommand.skipValidInstallCheck = false;

packages/cli/src/oclif/commands/scaffold.js
219:ScaffoldCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/integrations.js
32:IntegrationsCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/analytics.js
45:AnalyticsCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/login.js
148:LoginCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/init.js
53:InitCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/logout.js
35:LogoutCommand.skipValidInstallCheck = true;

packages/cli/src/oclif/commands/convert.js
132:ConvertCommand.skipValidInstallCheck = true;

So it's easier than we thought. You only need to set skipValidInstallCheck to true for the commands that don't need core installation. Sorry for not seeing this flag earlier! 🙇

[robwa10]

@eliangcs great catch, I should have seen that earlier as well! But at least it helped me learn how to do what I wanted to do in the first place. I've made the changes to add that flag to all the files that need it.

[eliangcs]

I'm pretty sure zapier test and zapier validate (and perhaps zapier build) require core installation, so their skipValidInstallCheck should be false. And commands like zapier analytics and zapier team:* don't require core installation. Can you double check that you have the right list of commands? Thanks!

[robwa10]

@eliangcs sorry about missing the commands in the folders (delete, team, etc) 🤦 . What I did this time to test was:

1. Set skipValidInstallCheck in all the commands to true.
2. Made sure I didn't have zapier-platform-core installed in a local test cli app.
3. Ran all the commands one by one, setting skipValidInstallCheck back to true for any command that threw an error related to not having core installed.

I couldn't think of another way to test the commands to be sure if they needed core or not. If you've got any suggestions on other ways let me know! 👍 :ty:

[eliangcs]

I also did the same for all the commands and got the same results as yours. Posting here for clarity:

command             skipValidInstallCheck
-----------------------------------------------------------------------------------------
analytics           true
build               false  // depends on `validate`, which requires core
convert             true
delete:integration  true
delete:version      true
deprecate           true
describe            false  // depends on `definition` local command to get app definition
env:get             true
env:set             true
env:unset           true
history             true
init                true
integrations        true
jobs                true
link                true
login               true
logout              true
logs                true
migrate             true
promote             true
push                false  // depends on `validate`, which requires core
register            true
scaffold            true
team:add            true
team:get            true
team:remove         true
test                false  // "Cannot find module 'zapier-platform-core'" when skipped
upload              true   // works as long as build.zip and source.zip exist
users:add           true
users:get           true
users:links         true
users:remove        true
validate            false  // "Cannot find module 'zapier-platform-core'" when skipped
versions            true

I pushed a commit to remove the skipValidInstallCheck = false statements. They're unnecessary because the default is already false. Also did some cleaning while I was there. If the changes look good to you, please go ahead and merge it. Thanks!