Skip to content

Remove EnvelopesQuery, use hooks over props for app components #374

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

Merged
merged 9 commits into from
Apr 5, 2025

Conversation

davidjgoss
Copy link
Contributor

@davidjgoss davidjgoss commented Apr 5, 2025

🤔 What's changed?

This PR makes some changes to the library to simplify things for consuming apps, and simplify some internals too.

  • With our dependencies updated, we can now remove the EnvelopesQuery class since recent additions to @cucumber/query give us what we need for those use cases. A couple of other helpers are also defunct and duly removed for the same reason.
  • Previously it was more difficult than it needed to be to compose our "app" components (<StatusesSummary/>, <ExecutionSummary/>, <SearchBar/>) together to make the kind of report you want - it required a lot of orchestration and understanding to pass the required props to each component and interact with the right bits of context. Now those components are prop-free, using hooks internally to get and set the state they need, and the FilteredResults component is now an exercise in straightforward composition. This isn't the trade off we'd always make for e.g. a general purpose component library, but here it makes sense to bake in the assumption that you're working within an <EnvelopesWrapper/> context and reap the DX benefits.

Since many of the changed components were exported, this is a breaking change.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • 💥 Breaking change (incompatible changes to the API)

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@davidjgoss davidjgoss marked this pull request as ready for review April 5, 2025 09:20
Copy link

netlify bot commented Apr 5, 2025

Deploy Preview for cucumber-react-preview ready!

Name Link
🔨 Latest commit 8dbecdc
🔍 Latest deploy log https://app.netlify.com/sites/cucumber-react-preview/deploys/67f0fd4cf35a550008b82f2a
😎 Deploy Preview https://deploy-preview-374--cucumber-react-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@davidjgoss davidjgoss requested a review from Copilot April 5, 2025 09:48
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 34 out of 35 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • package.json: Language not supported
Comments suppressed due to low confidence (3)

src/components/gherkin/MDG.spec.tsx:29

  • Since EnvelopesQuery has been removed, verify that tests and logic relying on its updates are fully updated to work without envelope state propagation.
for (const envelope of envelopes) { ... envelopesQuery.update(envelope) }

src/components/app/SearchBar.spec.tsx:163

  • [nitpick] The checkbox label 'undefined' may be unclear; please verify if this status label is intentional or if a more descriptive name should be used.
await userEvent.click(getByRole('checkbox', { name: 'undefined' }))

src/components/app/ExecutionSummary.tsx:26

  • [nitpick] Consider incorporating the 'passed' label directly within the formatPassRate function to centralize the formatting logic.
formatPassRate(scenarioCountByStatus[TestStepResultStatus.PASSED], totalScenarioCount) + ' passed'

@davidjgoss davidjgoss merged commit 682f9b4 into main Apr 5, 2025
6 checks passed
@davidjgoss davidjgoss deleted the app-refactor branch April 5, 2025 15:07
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