Skip to content
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

fix(remix): move @sentry/cli to optional dependency #9740

Closed
wants to merge 1 commit into from

Conversation

Mause
Copy link

@Mause Mause commented Dec 5, 2023

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Happy to add a changelog entry for this too

@AbhiPrasad
Copy link
Member

Hey @Mause thanks for opening a PR! What is the reason behind this move?

We recently changed @sentry/cli to use binaries over npm instead of a postinstall script: getsentry/sentry-cli#1836 - would that help this?

@Mause
Copy link
Author

Mause commented Dec 5, 2023

Hey @Mause thanks for opening a PR! What is the reason behind this move?

We recently changed @sentry/cli to use binaries over npm instead of a postinstall script: getsentry/sentry-cli#1836 - would that help this?

I assume installation still hard fails on platforms without pre-provided binaries? That's my reason for this change

@AbhiPrasad
Copy link
Member

I opened #9741 for the Sentry CLI bump - this should have better behavior and not rely on postinstall.

@AbhiPrasad
Copy link
Member

I assume installation still hard fails on platforms without pre-provided binaries

What platform are you using? Perhaps we can think about extending support to it.

@Mause
Copy link
Author

Mause commented Dec 5, 2023

I assume installation still hard fails on platforms without pre-provided binaries

What platform are you using? Perhaps we can think about extending support to it.

On Android via Termux (https://termux.dev/en/). I'm aware this is an oddball platform, but hard failing on installing a package that isn't required isn't a great experience

@Mause
Copy link
Author

Mause commented Dec 5, 2023

I've been overriding the resolution in yarn to a dummy empty package to work around this for now: https://github.com/Mause/github-statuses/blob/78f3fa4cef275e0bd34eb3d4a7e3a6d90a4217ca/package.json#L92

@AbhiPrasad
Copy link
Member

I'm aware this is an oddball platform, but hard failing on installing a package that isn't required isn't a great experience

I agree it's poor UX that it hard fails. Hopefully the CLI bump in #9741 makes this better.

I've been overriding the resolution in yarn to a dummy empty package to work around this for now

What happens if you use @sentry/cli@2.22.3?

@Mause
Copy link
Author

Mause commented Dec 5, 2023

I'm aware this is an oddball platform, but hard failing on installing a package that isn't required isn't a great experience

I agree it's poor UX that it hard fails. Hopefully the CLI bump in #9741 makes this better.

I've been overriding the resolution in yarn to a dummy empty package to work around this for now

What happens if you use @sentry/cli@2.22.3?

Looks like that does the trick - it's able to install, and the sentry cli binary just fail on invocation (with https://github.com/getsentry/sentry-cli/blob/c09c5991fd44b8fa4a0efff39b6953f98892e013/js/helper.js#L85), which I'm perfectly happy with

@Mause Mause closed this Dec 5, 2023
@Mause
Copy link
Author

Mause commented Dec 5, 2023

chore: remove sentry cli hack 😁

@AbhiPrasad
Copy link
Member

@Mause updated CLI bump will go out with the next version of the SDK. I also opened a PR to configure dependabot so we don't get caught in situations like this in future #9752

Thanks again for opening PR!! Was great reminder for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants