Skip to content

feat(nextjs): Allow custom path to sentry.client.config.ts #4187

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

Closed
wants to merge 2 commits into from

Conversation

igrayson
Copy link
Contributor

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

This permits us to store our sentry.*.config.ts files somewhere other than the current directory, and therefore share sentry.*.config.ts between projects.

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

@igrayson igrayson changed the title Allow custom path to sentry.client.config.ts feat(nextjs): Allow custom path to sentry.client.config.ts Nov 27, 2021
@AbhiPrasad
Copy link
Member

Hey thanks for opening a PR. We currently don't have the bandwidth to review it atm, but will send an update when we do.

@igrayson
Copy link
Contributor Author

igrayson commented Dec 8, 2021

Hi @AbhiPrasad . Do you know when this will make it in the review queue? We're depending on this before rolling out sentry/nextjs broadly.

@AbhiPrasad
Copy link
Member

Hey we are still evaluating if we want to add the option or not. We will update this PR as soon as we make a decision!

@lobsterkatie
Copy link
Member

lobsterkatie commented Dec 8, 2021

Hi, @igrayson.

Thanks for the PR! We talked it over, and we've decided not to add this to the SDK, for a few reasons:

  • In general we're wary of adding new options unless they solve a problem which can't be solved any other way, because even the simplest-seeming option adds incrementally to the complexity of the SDK code, the maintenance/support burden on our end, and the ease of use (or lack thereof) for the average SDK user.

  • In this case, one of the problems this PR would solve - decluttering your project directory - can be solved with already-existing options. (There is an issue addressing that question, where I've posted some suggestions.) In your case, I'd suggest storing the options passed to Sentry.init in a shared location instead, so that your sentry.server.config.js file might look like:

    import * as Sentry from '@sentry/nextjs';
    import { sentryServerOptions } from 'my/shared/location`;
    
    Sentry.init({
      ...sentryServerOptions,
      // app-specific options
    });
    
  • Finally, making this work isn't quite as simple as the changes represented in this PR. (As written, it introduces inconsistency between absolute paths, relative paths, and bare filenames.) Nothing impossible to fix, but enough logic that it should probably be pulled into getUserConfigFile, which then changes that function's signature, which requires changes to a number of tests, as well as versions of all of those tests incorporating this new option (or refactoring those tests to be parameterized)... at which point we hit the incremental complexity question from point 1.

We really do appreciate the contribution, though, and please feel free to make others in the future!

@igrayson
Copy link
Contributor Author

igrayson commented Dec 8, 2021

Hmm, okay — thanks for explaining your position.

For the next time this comes under review, this is our position:

  • in our view, this currently cannot be solved any other way (without forking the SDK), because:

  • the goal is indeed to declutter our project directory, and we disagree that it can be solved with another option. In the issue you've linked you state "sentry.server.config.js and sentry.client.config.js need to remain at the root level of your project." — i.e. the clutter remains.

  • re: path inconsistencies. We'd love to understand more on your concern on this. In the spirit of collaboration, perhaps we could together arrive at a solution that doesn't add the potential complexity problem that you've raise.

Cheers!

(@lobsterkatie @AbhiPrasad)

@ozyman42
Copy link

In this case, one of the problems this PR would solve - decluttering your project directory - can be solved with already-existing options.

That is not a correct statement @lobsterkatie

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 21, 2023

For those following this PR, I left some extra context in this GH issue: #4249 (comment). We can continue the conversation there!

@getsentry getsentry locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Package: nextjs Issues related to the Sentry Nextjs SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants