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

[20] Fix NextJS example "showExperiments" query string and update contributing guide #21

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

Conversation

TomStrepsil
Copy link
Contributor

@TomStrepsil TomStrepsil commented Feb 7, 2025

Issue

20

Details

This PR fixes the issue whereby the ?showExperiments querystring handling in the NextJs example is ineffective.

Originally, the NextJs example demonstrated the ssr package, despite this package being superfluous for a platform that had native props-serialization. When an opportunity arose to demonstrate this package elsewhere (made in v0.2.0), the NextJs example was moved to more canonical approach for Next. However, the debug querystring to make the <script type="application/json"> inserted by ssr visible would no longer do anything. This PR fixes that mistake.

After a failed check run its evident that the Linux Playwright screenshot tests can produce false negatives. Imperceptible changes (but with changes to the binary content) are being flagged. May need an issue to address, via playwright settings? For now, the PR has updated snapshots.

Scout Rule:

  • Fix some links to the packages from the express examples README.md, which had missed a directory level
  • Clarify the need to fork the repository, or get given write access (i.e. ASOS internal), before contributing PRs, as per GitHub documentation:

    If you want to create a new branch for your pull request and do not have write permissions to the repository, you can fork the repository first.

  • Updated the version of danger to attempt to get around a failing job in the check run - this had masked a genuine danger failure, now fixed. But the lack of feedback is a rabbit hole. See linked issue
    • to be clear, the "Resource not accessible by integration" is a known issue (plus this)

CheckList

  • PR starts with [ISSUE_ID].
  • Has been tested (where required) before merge to main.

@@ -1,6 +1,6 @@
{
"name": "web-toggle-point-serve-example",
"version": "0.2.0",
"version": "0.2.3",
Copy link
Contributor Author

@TomStrepsil TomStrepsil Feb 9, 2025

Choose a reason for hiding this comment

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

N.B. prior version was errant, missed in refix of the versioning for OSS release. Should have matched Changelog. Probably need a new danger rule to ensure a match (or perhaps adoption of https://github.com/release-it/keep-a-changelog (perhaps not tied to "release-it"), https://www.npmjs.com/package/@firefoxic/update-changelog, or similar - as long as these work for monorepos)

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.

1 participant