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

Use async file operations for helpers #2861

Merged
merged 8 commits into from
Oct 6, 2022
Merged

Use async file operations for helpers #2861

merged 8 commits into from
Oct 6, 2022

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Sep 20, 2022

This PR has now been split into two

Part 1#2897 Spruce up Puppeteer usage to stop timeouts, page conflicts etc
Part 2#2861 Use async file operations for helpers (this one)

For context, we "discover" components, examples and full page examples via directory lookups. But we also parse *.yaml files per component, building "component data" for Nunjucks macro options, examples, layouts and accessibility criteria

These are all tiny operations that quickly add up

Problem areas

  1. Requesting review app pages hits *.yaml files (and directory lookups) repeatedly
  2. Multiple ways to hit *.yaml files via getExamples() or getComponentData()
  3. Multiple ways to check for JavaScript support via readdir() or glob() filtering

Changes to file system access

All of our test helpers are now asynchronous using the promise based Node.js file system methods

readdirSync()readdir()
readFileSync()readFile()
statSync()stat()

Changes to test helper naming conventions

For consistency I've also ensured component/componentName/exampleName naming:

const component = await getComponent(componentName)

component.name
component.params
component.previewLayout
component.accessibilityCriteria

Plus the JSDoc to go with it (tiny addition, but is quite helpful)

/**
 * Component data from YAML
 *
 * @typedef {object} ComponentData
 * @property {string} name - Component name
 * @property {unknown[]} [params] - Nunjucks macro options
 * @property {unknown[]} [examples] - Example Nunjucks macro options
 * @property {string} [previewLayout] - Nunjucks layout for component preview
 * @property {string} [accessibilityCriteria] - Accessibility criteria
 */

JSDoc @typedef showing all available component properties

@colinrotherham colinrotherham requested a review from a team as a code owner September 20, 2022 08:11
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 20, 2022 08:11 Inactive
@colinrotherham colinrotherham marked this pull request as draft September 20, 2022 08:17
@colinrotherham
Copy link
Contributor Author

Converting to draft whilst I look at the various timeouts. Unblocking IO may have made Jest or Puppeteer trip over

Base automatically changed from upgrade-test-jest-percy to main September 20, 2022 09:11
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 20, 2022 19:10 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 20, 2022 19:56 Inactive
@colinrotherham colinrotherham changed the base branch from main to lint-separately September 20, 2022 19:56
@colinrotherham colinrotherham marked this pull request as ready for review September 20, 2022 19:59
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 20, 2022 20:06 Inactive
@colinrotherham colinrotherham force-pushed the lint-separately branch 4 times, most recently from 0dddc0d to 5ae1389 Compare September 22, 2022 11:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 22, 2022 15:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 September 22, 2022 15:54 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 October 5, 2022 15:14 Inactive
@romaricpascal
Copy link
Member

Thanks for splitting the commits and going through all these changes. The move to async looks good to me, I like how it feels more parallel-y with the Promise.all 🙌🏻 .

Only concern I have in that PR is the naming move from componentData to component that:

  • looses the separation between the data about the component and the component itself
  • makes it weird with the types of a lot of component variables being ComponentData

It feels a bit nitpicky and I'm not holding strongly to going back to componentData, but thought I'd mention it 😄

@colinrotherham
Copy link
Contributor Author

Ha, that's fine

Consistency wise, I'm more than happy to make everything componentData

Consistency was the aim, not the name 🙌

Or rename the @typedef to Component?

@romaricpascal
Copy link
Member

Making everything componentData makes more sense to me, as we keep the distinction from the component itself (in case we ever have tests that need to reference both).

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 October 6, 2022 07:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 October 6, 2022 07:12 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Oct 6, 2022

Making everything componentData makes more sense to me, as we keep the distinction from the component itself (in case we ever have tests that need to reference both).

@romaricpascal All sorted, now using componentData instead 🎉

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Ace! Looks all good to me, thanks for that extra pass on it 😄

Base automatically changed from puppeteer-page-object to main October 6, 2022 10:39
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 October 6, 2022 10:47 Inactive
Now cached, ensures `*.yaml` files trigger Node.js restart via nodemon
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2861 October 6, 2022 10:48 Inactive
@colinrotherham colinrotherham merged commit f0904eb into main Oct 6, 2022
@colinrotherham colinrotherham deleted the async-helpers branch October 6, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

4 participants