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

Stencil v4 upgrade #1304

Merged
merged 16 commits into from
Sep 4, 2024
Merged

Stencil v4 upgrade #1304

merged 16 commits into from
Sep 4, 2024

Conversation

powellkerry
Copy link
Contributor

@powellkerry powellkerry commented Aug 27, 2024

Chromatic

https://stencil-v4-upgrade--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

Upgrade stencil to v4
Closes 2233

QA Checklist

  • Component maintains 1:1 parity with design mocks
  • Text is consistent with what's been provided in the mocks
  • Component behaves as expected across breakpoints
  • Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
  • Designer has signed off on changes (if applicable. If not applicable provide reason why)
  • Tab order and focus state work as expected
  • Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
  • New components are covered by e2e tests; updates to existing components are covered by existing test suite
  • Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

No visual changes

Acceptance criteria

  • QA checklist has been completed
  • Screenshots have been attached that cover desktop and mobile screens

Definition of done

  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

@powellkerry powellkerry added the minor For a minor Semantic Version change label Aug 27, 2024
@powellkerry powellkerry marked this pull request as ready for review August 28, 2024 17:22
@powellkerry powellkerry requested a review from a team as a code owner August 28, 2024 17:22
@@ -28,7 +28,7 @@
},
"dependencies": {
"@department-of-veterans-affairs/css-library": "^0.8.1",
"@stencil/core": "^3.2.1",
"@stencil/core": "4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to open pandora's box but did you happen to try to latest release of v4? It looks like they're up to v4.21 and v4.0.0 is not actively maintained.

Copy link
Contributor Author

@powellkerry powellkerry Aug 28, 2024

Choose a reason for hiding this comment

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

The later versions have issues with the piercing (>>>) selector in the e2e tests. I have checked the documentation and the how-to pages and they all say that this should still work in 4.21 but they don't. This is the error I get when running tests using 4.21:

Evaluation failed: DOMException: Failed to execute 'querySelector' on 'Document': 'va-select >>> select' is not a valid selector.

It results in 277 failed tests.

Copy link
Contributor

@jamigibbs jamigibbs Aug 28, 2024

Choose a reason for hiding this comment

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

Interesting. I guess that makes wonder a few more things:

  1. Are we ok just stopping here for now (like what does being on v4.0.0 unblock for us now?) or is stopping here just delaying us eventually needing to solve for it?
  2. Should we create a follow-up ticket to get us onto the latest (or a more recent) version?

Glancing at their release notes, version 4.14.0 seems particularly useful for testing because they added support for deep piercing with Puppeteer. I know we've had some problems dealing with this in our tests and we had testing gaps because of not being able to do that.

There are probably other useful things too I just haven't really dug into what else they've added.

Copy link
Contributor

@jamigibbs jamigibbs Aug 28, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been trying to get this to work locally but haven't succeeded yet. Trying to see what happens when jest is running on the latest version in addition to stencil 4.21.0. Currently getting some weird errors :\

image

Copy link
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

This is an exciting update! Nice work!

@@ -27,7 +27,7 @@ jobs:
- run: yarn workspace @department-of-veterans-affairs/web-components run build
- run: yarn workspace @department-of-veterans-affairs/component-library run build
- name: Publish to Chromatic
uses: chromaui/action@v1
uses: chromaui/action@latest
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was done just to try and get the chromatic action to work again (unrelated to the v4 upgrade itself). But I guess if it's working still maybe it can stay as it is?

Copy link
Contributor Author

@powellkerry powellkerry Sep 4, 2024

Choose a reason for hiding this comment

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

I reverted that. @latest seems a little dangerous.

const inputEl = await page.$('pierce/[name="veteran-signature"]');
const value = await page.evaluate(element => element.value, inputEl);

const value = await page.$eval('va-statement-of-truth >>> va-text-input >>> input', (comp: HTMLInputElement) => comp.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so excited to have deep piercing now 🤗

Copy link
Contributor

@micahchiang micahchiang left a comment

Choose a reason for hiding this comment

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

🚀

@powellkerry powellkerry merged commit c56f2f4 into main Sep 4, 2024
8 checks passed
@powellkerry powellkerry deleted the stencil-v4-upgrade branch September 4, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor For a minor Semantic Version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency Management - Upgrade Stencil to version 4
3 participants