Skip to content

USWDS - Dependencies: Add override for nwsapi#6249

Merged
thisisdano merged 1 commit into
developfrom
cm-override-nwsapi
Dec 16, 2024
Merged

USWDS - Dependencies: Add override for nwsapi#6249
thisisdano merged 1 commit into
developfrom
cm-override-nwsapi

Conversation

@mahoneycm

@mahoneycm mahoneycm commented Dec 11, 2024 •
edited by cathybaptista
Loading

Copy link
Copy Markdown
Contributor

Summary

Added override for nswsapi@2.2.13. Now, the nested dependency will not cause unit tests to fail.

Breaking change

This is not a breaking change

Related issue

Closes #6242

Related pull requests

N/A

Preview link

Preview link →

Problem statement

nwsapi recently pushed out version 2.2.16 which unintentionally introduced breaking changes to jsdom. Despite not having the nwsapi dependency and not updating jsdom this update is affecting our unit tests.

Additional information in dperini/nwsapi#135

Solution

Override nwsapi version for our dependencies until another fix has been identified.

Alternative solutions considered

  • Remove : selector from our unit tests
  • Try to find a version of jsdom that is unaffected by this change
  • Install nwsapi@2.2.13 as a dependency

Major changes

  • Unit tests and pipeline failures will be resolved as part of this PR.

Testing and review

  1. Confirm CI tests are passing

  2. Checkout this branch

  3. Remove node_modules and package-lock.json files

    rm -rf node_modules package-lock.json

  4. Run npm install

  5. Run npm run test:unit

  6. Confirm there are no installation or test errors

  7. Run npm ls nwsapi

  8. Confirm the only listing is a sub dependency of jsdom

  9. Confirm the version listed is 2.2.13 overridden

Testing checklist

  • No installation errors
  • Unit tests pass as expected
  • CI tests are passing
  • Approach is considered appropriate

@mahoneycm mahoneycm requested a review from a team as a code owner December 11, 2024 16:38
@amyleadem amyleadem mentioned this pull request Dec 11, 2024
Merged
21 tasks
amyleadem
amyleadem approved these changes Dec 11, 2024
View reviewed changes

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM - Thanks for this fix @mahoneycm.

  • Confirm that failing unit tests should pass
    • Removing the responsive utility classes (like desktop:grid-col-3) does allow the unit tests to pass without this override update. These grid classes are not part of the functionality being checked in the unit tests, so I'm concluding that these failures are not related to the test content.
  • Confirm that unit tests fail with nwsapi v2.2.16 (most recent), 2.2.15, 2.2.14
  • Confirm that tests pass with nwsapi` v2.2.13

Possible follow-up

We should create an issue to follow up and see if we can remove this override in the future.

cathybaptista
cathybaptista reviewed Dec 11, 2024
View reviewed changes

@cathybaptista cathybaptista left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mahoneycm LGTM, except I am not 100% sure if the "approach is appropriate." Seems to work as designed as far as I can tell, and will leave it to others to determine the "appropriateness" of the approach. :)

@cathybaptista cathybaptista self-requested a review December 11, 2024 21:54
@mahoneycm mahoneycm mentioned this pull request Dec 11, 2024
Open
@mahoneycm

mahoneycm commented Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

Removing the responsive utility classes (like desktop:grid-col-3) does allow the unit tests to pass without this override update. These grid classes are not part of the functionality being checked in the unit tests, so I'm concluding that these failures are not related to the test content.

@amyleadem thanks for flagging this! I originally thought we needed these styles to get the big-footer accordions to display correctly but it looks like we don't!

I opened #6250 to keep track of a longer-term solution if nwsapi doesn't resolve this in time. While it's not ideal to have the test template not match the component's expected markup, it's good to know that's an option!

@thisisdano thisisdano merged commit eedb330 into develop Dec 16, 2024
@thisisdano thisisdano deleted the cm-override-nwsapi branch December 16, 2024 18:46
@amyleadem amyleadem mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mandylloyd mandylloyd Awaiting requested review from mandylloyd
@CTGM-Bixal CTGM-Bixal Awaiting requested review from CTGM-Bixal
@heymatthenry heymatthenry Awaiting requested review from heymatthenry
3 more reviewers
@thisisdano thisisdano thisisdano approved these changes
@amyleadem amyleadem amyleadem approved these changes
@cathybaptista cathybaptista cathybaptista approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

USWDS - Bug: jsdom is causing unit test failures due to nwsapi update

4 participants

@mahoneycm @thisisdano @amyleadem @cathybaptista