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

Unify setting the preview mode for getExperiments #5575

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Feb 3, 2025

References:

Jira: MNTOR-4011

Description

At the moment, we have two ways of forcing experiments via nimbus_preview:

  1. Via the search param nimbus_preview in pages
  2. Via x-nimbus-preview-mode in layouts (which is getting set by the search param nimbus_preview in the middleware)

This PR unifies the the Nimbus preview to check x-nimbus-preview-mode only and moves the condition into getExperiments.

How to test

Append /?nimbus_preview to a URL that has disabled experiments.

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • If this PR implements a feature flag or experimentation, I've checked that it still works with the flag both on, and with the flag off.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Copy link

github-actions bot commented Feb 3, 2025

@flozia flozia self-assigned this Feb 3, 2025
@flozia flozia requested review from rhelmer and Vinnl and removed request for rhelmer February 3, 2025 14:25
Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Looks much more reliable this! If @rhelmer has time, I think it'd be good if he'd take a look as well, but I'm fairly confident that this is safe and right.

Comment on lines +56 to +57
// Check for Nimbus preview mode. Note that this requires a full page reload
// to activate: https://nextjs.org/docs/app/api-reference/file-conventions/layout#caveats
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment doesn't quite seem to apply here, though it could possibly reworded to talk about if getExperiments is called in a layout.

(That said, I'm not sure what the failure mode here is - surely the query parameter only gets added by manually adjusting the URL, in which case pressing Enter seems implied? We're not supposed to add <Link>s with these query params, right?)

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.

3 participants