Does it actually need to depend on sentry@cli?

[marandaneto]

sentry-wizard/lib/Helper/SentryCli.ts

Line 30 in 63cbb3f

const cliPath = this._resolve('@sentry/cli/bin/sentry-cli');

This only resolves the path but it does not need at runtime, the question is that Apps that have wizard as a dev dependency don't depend on cli directly, that would be breaking.
For RN, that would not be a problem, because RN itself depends on the cli, wondering if this is the same for every other SDK that depends on the wizard.

The problem is that RN is having a version mismatch of the cli with both dependencies (cli and wizard).

RN https://github.com/getsentry/sentry-react-native/blob/1f86e232bb159ac2311f8be0e4d4725cdb2265f8/package.json#L44

Relates to getsentry/sentry-react-native#2556

[lobsterkatie]

The only SDKs (in the getsentry org, at least) which have @sentry/wizard as a dependency are RN, Cordova, and Capacitor. Of those, only RN has @sentry/cli as an independent dependency: https://cs.github.com/?scopeName=All+repos&scope=&q=path%3Apackage.json+%22%40sentry%2Fwizard%22+org%3Agetsentry.

Honestly I think this is asking the wrong question, though. IMHO, the wizard itself shouldn't ever be a dependency, unless it's doing a task which occurs on an on-going basis. (And if it is, that task should be moved to sentry-cli.) Otherwise it should just be used through npx. So I think the question is, can it be removed from dependencies in those three SDKs?

[krystofwoldrich]

I think the question is valid, since @sentry/wizard doesn't use @sentry/cli, so why does it have as a dependency?

@sentry/react-native depends on @sentry/cli because it's used during the application build (debug files upload, source maps) and we want to keep the install process as simple as possible (We could install @sentry/cli using @sentry/wizard but that has its own issues).

The reason why @sentry/react-native@4 depends on @sentry/wizard is that the latest version of the @sentry/wizard is not compatible with @sentry/react-native@4 and npx by default executes commands found locally.

Either we need to keep some dependency relationship to show version compatibility or we can't make breaking changes (meaning a new version won't be able to patch older SDKs) in the wizard.

I'm definitely for removing @sentry/wizard and @sentry/cli as deps from all the projects, but it can't degrade users' experience.

[lobsterkatie]

The reason why @sentry/react-native@4 depends on @sentry/wizard is that the latest version of the @sentry/wizard is not compatible with @sentry/react-native@4 and npx by default executes commands found locally.

Ah, gotcha.

I think the question is valid, since @sentry/wizard doesn't use @sentry/cli, so why does it have as a dependency?

So, mea culpa, I didn't get as far as seeing what @sentry/cli actually does in the wizard, my brain just jumped to, "Hey, wait, the wizard shouldn't be a dependency!" I've now done so and indeed, the bit Manoel quoted initially is the only use - finding the path to the executable for the sentry.properties file. Of course, kinda by definition, if you care about the path to the cli binary, you're a package which uses it, and it should be one of your dependencies. (Meaning, RN has it right, the rest don't.)

Here'd be my proposal:

• Leave the wizard as a dependency of RN (for now, forever... I'll leave that in your court)
• Remove @sentry/cli from the wizard's dependencies
• Make it a real dependency in cordova (and maybe capacitor? Though capacitor isn't actually mentioned in the wizard anywhere, so not at all clear why it has the wizard as a dependency)
• If the wizard goes to look for sentry-cli and it's not there (because, say, the wizard is being run on an older version of one of those SDKs), then we error out and tell people to add it as a dependency themselves (basically, we're making it a peer dependency, but an enforced one)

Thoughts?

[krystofwoldrich]

• Leave the wizard as a dependency of RN (for now, forever... I'll leave that in your court)

Yes, make sense in our case, we'll leave it for v4 and remove it in the future v5.

• Remove @sentry/cli from the wizard's dependencies

I agree.

• Make it a real dependency in cordova (and maybe capacitor? Though capacitor isn't actually mentioned in the wizard anywhere, so not at all clear why it has the wizard as a dependency)

Maybe. I would leave it up to the maintainers if they are fine with using the latest wizard or add it as dep and use a specific version. The same as with RN SDK.

• If the wizard goes to look for sentry-cli and it's not there (because, say, the wizard is being run on an older version of one of those SDKs), then we error out and tell people to add it as a dependency themselves (basically, we're making it a peer dependency, but an enforced one)

I would not enforce it, right now if the path is not found it just doesn't create the entry in sentry.properties. I honestly need more information on how is this path used and where to have an opinion. Because the properties file is read by the CLI so we need to know the path before so I'm missing the purpose of this information.

I can imagine this being used in the templates to make the wizard patches dynamic and auto-select the path to cli. But I haven't seen any use of the path in the wizard.

[lobsterkatie]

• Make it a real dependency in cordova (and maybe capacitor? Though capacitor isn't actually mentioned in the wizard anywhere, so not at all clear why it has the wizard as a dependency)

Maybe. I would leave it up to the maintainers if they are fine with using the latest wizard or add it as dep and use a specific version. The same as with RN SDK.

With all due respect, I disagree with this. If you're using a package at runtime, it should be in your dependencies. Period. A one-time helper package which isn't necessary to the functioning of your library shouldn't. (Like I said, I get the reason why for now RN is set up the way it is, but we don't want to move more of our SDKs to the wrong setup.)

• If the wizard goes to look for sentry-cli and it's not there (because, say, the wizard is being run on an older version of one of those SDKs), then we error out and tell people to add it as a dependency themselves (basically, we're making it a peer dependency, but an enforced one)

I would not enforce it, right now if the path is not found it just doesn't create the entry in sentry.properties. I honestly need more information on how is this path used and where to have an opinion. Because the properties file is read by the CLI so we need to know the path before so I'm missing the purpose of this information.

I can imagine this being used in the templates to make the wizard patches dynamic and auto-select the path to cli. But I haven't seen any use of the path in the wizard.

It's not really about the path, though, or the properties file. The point is, if you care about the path, it's because you're at some point going to be using the binary, so people need to have it installed, either automatically by dint of it being a dependency or manually because we yell at them to add it.

[krystofwoldrich]

• Make it a real dependency in cordova (and maybe capacitor? Though capacitor isn't actually mentioned in the wizard anywhere, so not at all clear why it has the wizard as a dependency)

Maybe. I would leave it up to the maintainers if they are fine with using the latest wizard or add it as dep and use a specific version. The same as with RN SDK.

With all due respect, I disagree with this. If you're using a package at runtime, it should be in your dependencies. Period. A one-time helper package which isn't necessary to the functioning of your library shouldn't. (Like I said, I get the reason why for now RN is set up the way it is, but we don't want to move more of our SDKs to the wrong setup.)

I absolutely agree. Maybe I misunderstood what you meant by Make it a real dependency in cordova because the wizard is a "real dep" now and in my opinion it should not be if it is not a similar case to RN.

• If the wizard goes to look for sentry-cli and it's not there (because, say, the wizard is being run on an older version of one of those SDKs), then we error out and tell people to add it as a dependency themselves (basically, we're making it a peer dependency, but an enforced one)

I would not enforce it, right now if the path is not found it just doesn't create the entry in sentry.properties. I honestly need more information on how is this path used and where to have an opinion. Because the properties file is read by the CLI so we need to know the path before so I'm missing the purpose of this information.
I can imagine this being used in the templates to make the wizard patches dynamic and auto-select the path to cli. But I haven't seen any use of the path in the wizard.

It's not really about the path, though, or the properties file. The point is, if you care about the path, it's because you're at some point going to be using the binary, so people need to have it installed, either automatically by dint of it being a dependency or manually because we yell at them to add it.

If the CLI will be needed later then we should yell at them to add it or do it to ask if we can do it, but the cli should not be dep of the wizard as the wizard should not be dep of the SDKs. If the wizard is doing its magic for RN it should ensure that the cli will be present (install it or yell at the user).

Based on the usage of the cli.executable I assume that should stay.

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[krystofwoldrich]

I've opened a PR showing what I would change, happy to hear if it makes sense.
If it doesn't I will close this issue.

[krystofwoldrich]

Some SDKs are using a global installation of wizard and then the cli is used (not directly by the wizard but by the SDKs).