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

ref(nextjs): Add @sentry/cli as a dependency #5175

Closed
wants to merge 1 commit into from
Closed

ref(nextjs): Add @sentry/cli as a dependency #5175

wants to merge 1 commit into from

Conversation

kevinji
Copy link

@kevinji kevinji commented May 30, 2022

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).

Since @sentry/cli is included to grab its binary, I believe it should also be added explicitly as a dependency.

@AbhiPrasad
Copy link
Member

Hey, thanks for opening a PR! @sentry/cli is a dependency of the Sentry webpack plugin, which is a dependency on the NextJS package: https://github.com/getsentry/sentry-webpack-plugin/blob/db1475ef40e8ef7c8341c2878d51dbf4a3e134eb/package.json#L21. It is the webpack plugin that is grabbing the binary and using it.

As such, I think we do not need explicitly add @sentry/cli to our dependency list.

@AbhiPrasad AbhiPrasad closed this May 30, 2022
@kevinji
Copy link
Author

kevinji commented May 30, 2022

Gotcha, should it be added as an optional peerDependency then, like how webpack is added? Yarn PnP is unhappy that the nextjs package calls require.resolve('@sentry/cli') unless that's added.

Also, I force pushed a commit but not sure where it went; maybe I need to create a new PR if this one is closed?

@AbhiPrasad
Copy link
Member

Yarn PnP is unhappy that the nextjs package calls require.resolve('@sentry/cli')

This might be fixed in v7, can you give that a try and report back? #4882 (you'll also get some bundle size wins as a reward :) )

@kevinji
Copy link
Author

kevinji commented May 30, 2022

Still running into the same error in v7:

Error: @sentry/nextjs tried to access @sentry/cli, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: @sentry/cli
Required by: @sentry/nextjs@virtual:47772ab7c7059053605926617210fe2785345409fd135f03ed9af243268dc5833d9029118ee3ce335f653f9967b5898a7ad39a4cf9aa079aa83339f36ef2f816#npm:7.0.0

@danielchabr
Copy link

danielchabr commented Jul 7, 2022

If only @sentry/webpack-plugin accessed @sentry/cli then it would be fine but it seems that @sentry/nextjs tries to access @sentry/cli directly without having it listed in dependencies. Based on error stack trace the troublesome line of code is here: https://github.com/getsentry/sentry-javascript/blob/master/packages/nextjs/src/config/webpack.ts#L334

Until this is fixed properly you can hotfix it by adding these couple lines to yarnrc.yml:

packageExtensions:
  '@sentry/nextjs@*':
    dependencies:
      '@sentry/cli': '^1.74.4'

@AbhiPrasad Could this ticket be reopened? It's still happening even with 7.5.1 version

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.

None yet

3 participants