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

feat(nextjs): Add feature selection #631

Merged
merged 11 commits into from
Aug 7, 2024
Merged

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Jul 26, 2024

Part of: #622

This PR introduces:

  • Core feature selection logic, prompts and utils
  • Feature selection for NextJS wizard
  • NextJS SDK feature selection for:
    • Performance Monitoring
    • Session Replay
    • Spotlight (optionally generates config for it but it's still commented out like before) (Spotlight needs more setup and we'll do in a follow-up)

Copy link

github-actions bot commented Jul 26, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 85794d2

@onurtemizkan onurtemizkan force-pushed the onur/nextjs-feature-selection branch 2 times, most recently from 947aa1e to c7ef8a0 Compare July 30, 2024 12:56
@onurtemizkan onurtemizkan marked this pull request as ready for review July 30, 2024 13:14
@onurtemizkan onurtemizkan requested review from lforst and Lms24 and removed request for lforst July 30, 2024 13:14
@lforst lforst self-assigned this Jul 30, 2024
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Whoops I actually forgot to press submit on these comments, sorry for the delay!)

@@ -82,6 +98,20 @@ describe('NextJS code templates', () => {
// side errors will fail.
tunnelRoute: "/monitoring",

async headers() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this has to be put in the actual Next.js config - not the SDK config :/ But that can get very complicated, we could leave profiling out for now.

Comment on lines 1244 to 1253
export async function askShouldUseDefaulFeatureSet(): Promise<boolean> {
return traceStep('ask-use-default-feature-set', () =>
abortIfCancelled(
clack.confirm({
message: 'Do you want to use the default feature set?',
initialValue: true,
}),
),
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not give users this option! I think it is valuable for them to walk through what they are missing out on.

Comment on lines 52 to 68
export const NEXTJS_FEATURE_SET: Feature[] = [
{
id: 'performance',
name: 'Performance Monitoring',
},
{
id: 'replay',
name: 'Session Replay',
},
{
id: 'profiling',
name: 'Profiling',
},
{
id: 'spotlight',
name: 'Spotlight',
},
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One big part of giving users the option to select and deselct features is that we get to briefly explain these concepts to them, so ideally we give them short but valuable descriptions on all of these!

@onurtemizkan onurtemizkan force-pushed the onur/nextjs-feature-selection branch from 2e7b17f to 37eb3a2 Compare August 5, 2024 12:34
Comment on lines 52 to 67
{
id: 'performance',
name: 'Performance Monitoring',
description: 'Monitor your app performance and find bottlenecks.',
recommended: true,
},
{
id: 'replay',
name: 'Session Replay',
description: 'Replay user sessions to reproduce and fix bugs.',
},
{
id: 'spotlight',
name: 'Spotlight',
description: `Use Sentry's Spotlight debug tool (https://spotlightjs.com/) for your development workflow.`,
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lforst - Please feel free to update the descriptions I added here

@onurtemizkan onurtemizkan requested a review from lforst August 5, 2024 12:44
Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick pass, sorry to be so annoying about things. The wizard is a really really hot path in the onboarding and it needs to be bulletproof.


export async function featureSelectionPrompt(features: Feature[]) {
return traceStep('feature-selection', async () => {
return clack.multiselect({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to give every option its own yes/no step. 🤔 We can keep the features logic though, I like that!

id: 'performance',
name: 'Performance Monitoring',
description: 'Monitor your app performance and find bottlenecks.',
recommended: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if recommendations make to much sense 😄 technically all of the features are always recommended.

@onurtemizkan onurtemizkan requested a review from lforst August 6, 2024 09:53
@onurtemizkan onurtemizkan force-pushed the onur/nextjs-feature-selection branch 2 times, most recently from 863a13d to bdb6aa0 Compare August 7, 2024 13:15
@lforst lforst force-pushed the onur/nextjs-feature-selection branch from bdb6aa0 to 85794d2 Compare August 7, 2024 13:51
@lforst lforst merged commit c0c5462 into master Aug 7, 2024
14 checks passed
@lforst lforst deleted the onur/nextjs-feature-selection branch August 7, 2024 17:17
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