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

Migrate component data to ES modules #3812

Closed
wants to merge 5 commits into from
Closed

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jun 15, 2023

This PR migrates our component data YAML into ES modules to:

  1. Run type checks using TypeScript
  2. Run code quality checks using ESLint
  3. In future, import typed examples into tests without JSON conversion

This is important because we've accidentally missed things in the past:

But makes it a lot easier to add automated tests for:

Migration script

See branch component-migrator to convert YAML to ES modules via:

npm exec --workspace packages/govuk-frontend -- gulp fixtures

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 15, 2023 07:46 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 16, 2023 06:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 16, 2023 08:50 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 16, 2023 09:09 Inactive
@colinrotherham colinrotherham changed the title [WIP] Migrate component data to ES modules Migrate component data to ES modules Jun 16, 2023
@colinrotherham colinrotherham marked this pull request as ready for review June 16, 2023 09:20
@colinrotherham
Copy link
Contributor Author

Would appreciate your non-urgent thoughts on this one @36degrees

Using ES modules found some issues to fix so far:

  1. Missing property { required } in Checkboxes nested options for conditional fields
  2. Missing property { required } in Radios nested options for conditional fields
  3. Missing property { description } in Summary list 'key.html' row parameter
  4. Missing JSDoc type for example previewLayoutModifiers from #3808
  5. Missing JSDoc type for example description

That's before we've picked up:

@colinrotherham
Copy link
Contributor Author

Just rebasing since #2261 and tsc has caught another descrption typo in task-list.yaml

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 20, 2023 13:06 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 20, 2023 14:51 Inactive
@romaricpascal romaricpascal self-assigned this Jun 23, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 23, 2023 11:18 Inactive
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.

Sounds a neat way to catch errors, and gives us some nice way to catch which component options use specific fields (like isComponent). 😍

Screenshot 2023-06-23 at 13 14 21

No qualms on the naming which sounds fine to me (and nice to separate the params from the examples in different files as well).

Besides fixing the tests, Ollie suggested it could be good to present this to the whole team to take the temperature on not using YAML anymore, as it's a quite different authoring experience (especially for examples) 😊

shared/lib/index.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 June 26, 2023 08:55 Inactive
@colinrotherham colinrotherham changed the base branch from main to jest-dynamic-import June 26, 2023 08:56
@colinrotherham colinrotherham force-pushed the jest-dynamic-import branch 3 times, most recently from cb2624b to 11ec0f1 Compare June 26, 2023 10:28
Allows component data to run through ESLint and type checks
Component helpers like `getComponentNames()` use directory listings so we need to make sure the directory structure is created first
We now store component params as key/value objects just as we do for Nunjucks options, this improves access to values by param name
Component data YAML has now been replaced with ES modules
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3812 March 8, 2024 15:54 Inactive
@querkmachine
Copy link
Member

Closing for housekeeping and because there doesn't seem to be much appetite to pursue this at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants