-
-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@sentry/cli optional dependencies are not optional #1862
Comments
I think https://github.com/getsentry/sentry-react-native is the correct repro for this issue. Although at a quick glance I believe you are correct in that these are the CLI binaries. I'm not sure what would be a better option here. Maybe a postinstall: script that install one of these? I think people are even less likely to let those run. 🤔 Maybe use logic such as https://github.com/giggio/node-chromedriver, make it a required dependency, ant let it fetch the binary would be best. This in my personal account, and any comments made under it are not related to any company or project I may work for |
I'm not sure the Sentry RN repo is the right repo for this issue - if you had a plain node project that depended on @sentry/cli you'd hit the same issue - but it working something like the node-chromedriver example you posted looks like a good idea |
@liamjones, you are correct regarding the reason why these are marked as optional dependencies; it is because they are platform-specific. The dependency which contains the binary for your specific platform needs to be installed. Previously, we were using a post-install script to get the platform-specific binary, but we recently switched to using optional dependencies because the post-install script created issues for several people (see the issues linked on #1836 for context). If you cannot enable optional dependencies, perhaps you might be able to manually add the correct dependency containing the sentry-cli binary for your platform? |
I'm not sure we can do that, we have devs on multiple different platform architectures. Looks like I should be able to defer to the CLI installed on the host machine (e.g. via homebrew) which would already be platform appropriate? Line 78 in 54cde67
|
So in general, the thing I recommend doing is enabling optional dependencies. If that doesn't work for you, you can pin Sentry CLI to a version before 2.22.3. You could also run the install script that is contained in the package and provide a @drazisil is right. People are even less likely to enable postinstall scripts which is the alternative here. Long term we can think about adding back a postinstall script that will install the binary if we can't find optional dependencies but we will make the timeline around that dependent on how many people run into this. |
Thanks all, I've been able to able to change some things in our setup so we can re-enable optional dependencies for now.
@lforst regarding this point - do you want me to keep this issue open for other people who might run into it or close it since I've worked around the issue for our setup? |
@liamjones Feel free to keep it open. I think you're raising a good point that we need to provide an escape hatch for people who don't have support for optional dependencies. |
Another potential suggestion? If the dependency is missing and |
@liamjones Maybe yes, the issue I have with that is that it goes completely around the Node.js resolution algorithm and that this behavior would be very random. People could have different versions of the CLI installed globally (via brew for example) and it would be picked up by this mechanism. |
I agree with @lforst, let's keep this issue open in case others run into the same problem. If you encounter this same issue, please add a 👍 to this comment to help us gauge interest in creating an alternative installation mechanism for folks who cannot use optional dependencies. |
We had similar issue, but with different reason. We just integrated Sentry into our web app. I installed it via We figured out that when I installed Sentry on my Mac, only
See relevant issue on npm: npm/cli#4828 |
Just ran into this while trying to use |
I just had the same issue deploying to an Astro project with Sentry integration to Vercel. I develop on a Windows PC so the only binary installed was the windows binary but for whatever reason it doesn't work on Vercel. Not knowing which platform Vercel needs, I copied the whole block of optionalDependencies from @sentry/cli/package.json into my projects package.json, deleted node_modules and the lock file. Now I get the correct binary on dev and production. |
Which is fine for now but keep in mind that if you update the Sentry dependency and they change these optional dependency versions you'll also need to make sure you update your copied block to keep it in sync. |
(If you are encountering this issue please 👍 this comment below)
Environment
@sentry/react-native 5.15.1 (w/ @sentry/cli 2.23.0)
Project with optional dependencies excluded from our project for other reasons not related to Sentry (via .yarnrc
--install.ignore-optional true
)Steps to Reproduce
Tried to do a build after updating @sentry/react-native from 5.9.1 (w/ @sentry/cli 2.21.3)
Expected Result
iOS builds would work
Actual Result
iOS builds fail
Logs
Xcode build:
If they're required dependencies why are they in optional dependencies in the package.json? Something to do with them being platform specific I guess?
The text was updated successfully, but these errors were encountered: