vets-website icon indicating copy to clipboard operation
vets-website copied to clipboard

replace node-sass with sass

Open acald-creator opened this issue 1 year ago • 12 comments

Summary

vets-website has been using node-sass for quite some time, and it is time to replace node-sass with sass. Making this step will help us move forward and out of Node 14 and into a more recent version.

team: vsp-identity

  • [ ] update sass-loader to its latest api
  • [ ] update sass-loader options
  • [ ] use sass-migrator to migrate division and strict-unary
  • [ ] comment out @forward rule
  • [ ] remove rebuilding node-sass in scripts

Related issue(s)

  • Link to ticket created in va.gov-team repo department-of-veterans-affairs/va.gov-team#0000
  • Link to previous change of the code/bug (if applicable) department-of-veterans-affairs/vets-website#0000
  • Link to epic if not included in ticket department-of-veterans-affairs/va.gov-team#0000

Testing done

  • Describe what the old behavior was prior to the change
  • Describe the steps required to verify your changes are working as expected
  • Describe the tests completed and the results
  • _Exclusively stating 'Specs and automated tests passing' is NOT acceptable as appropriate testing
  • Optionally, provide a link to your test plan and test execution records

Screenshots

Note: This field is mandatory for UI changes (non-component work should NOT have screenshots).

Before After
Mobile
Desktop

What areas of the site does it impact?

(Describe what parts of the site are impacted if code touched other areas)

Acceptance criteria

Quality Assurance & Testing

  • [ ] I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • [ ] No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • [ ] Linting warnings have been addressed
  • [ ] Documentation has been updated (link to documentation *if necessary)
  • [ ] Screenshot of the developed feature is added
  • [ ] Accessibility testing has been performed

Error Handling

  • [ ] Browser console contains no warnings or errors.
  • [ ] Events are being sent to the appropriate logging solution
  • [ ] Feature/bug has a monitor built into Datadog or Grafana (if applicable)

Authentication

  • [ ] Did you login to a local build and verify all authenticated routes work as expected with a test user

:warning: Team Sites (only applies to modifications made to the VA.gov header) :warning:

  • [ ] The vets-website header does not contain any web-components
  • [ ] I used the proxy-rewrite steps to test the injected header scenario
  • [ ] I reached out in the #sitewide-public-websites Slack channel for questions

Requested Feedback

(OPTIONAL) What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

acald-creator avatar Jan 09 '24 14:01 acald-creator

@acald-creator I was able to build successfully with node 14 & node 16.18.1. I did run into errors building higher than 16 (I used 18.18.0 and 19.6.0) so that could possible spawn another ticket for investigation and/or a fix

ERROR in ./src/platform/site-wide/wc-loader.js <- there's 109 errors total across vets-website, 
Module build failed (from ./node_modules/babel-loader/lib/index.js):
Error: error:0308010C:digital envelope routines::unsupported

asg5704 avatar Jan 09 '24 17:01 asg5704

@acald-creator I was able to build successfully with node 14 & node 16.18.1. I did run into errors building higher than 16 (I used 18.18.0 and 19.6.0) so that could possible spawn another ticket for investigation and/or a fix

ERROR in ./src/platform/site-wide/wc-loader.js <- there's 109 errors total across vets-website, 
Module build failed (from ./node_modules/babel-loader/lib/index.js):
Error: error:0308010C:digital envelope routines::unsupported

Already have an open ticket on this to track the error. Open ticket here has been updated to reflect this: https://app.zenhub.com/workspaces/identity-5f5bab705a94c9001ba33734/issues/gh/department-of-veterans-affairs/va.gov-team/68616

acald-creator avatar Jan 09 '24 17:01 acald-creator

Due to the magnitude of what's being updated here, we're going to definitely want to review this one closely in a review instance. Additionally, I'd like to request that in addition to the standard FE COP review, that we have someone from the design system team review this as well to ensure that no components were unintentionally impacted.

CBonade avatar Jan 09 '24 21:01 CBonade

:wave: Hey there! I'm one of the engineers on the Design System Team. I think it's great y'all are working on this. I'm not sure if you've seen the prior discovery conducted on this particular issue, and in case you haven't there are two pieces:

  1. https://github.com/department-of-veterans-affairs/va.gov-team/issues/37351.
  2. Analysis on vets-website adjacent repos

Just want to make sure your team has as much historical context as possible for this. Let me know if there's anything the design system team can do to assist.

micahchiang avatar Jan 09 '24 21:01 micahchiang

👋 Hey there! I'm one of the engineers on the Design System Team. I think it's great y'all are working on this. I'm not sure if you've seen the prior discovery conducted on this particular issue, and in case you haven't there are two pieces:

  1. Upgrade sass processing in vets-website va.gov-team#37351.
  2. Analysis on vets-website adjacent repos

Just want to make sure your team has as much historical context as possible for this. Let me know if there's anything the design system team can do to assist.

I'm aware of the first open issue.

The only change that I believe could impact is the @forward rule, and while this is commented out in the meantime, because building with this on, will not work. It is necessary to upgrade USWDS to v3 in order to even use this rule. As I can see that there seems to be workarounds, this line is commented out for the time being, as there could be plans in the future to migrate to USWDS 3 internally in this repo, as well as formation.

There is no change to Foundation Sites and USWDS at this time. I've attempted to do so in a separate test repo, but found it was too cumbersome, and saw the latest work from the component library which made me rethink the approach.

I also intend to upgrade the node engine as well to at least v16, but not v18, as there are other issues with babel and other packages that needs to be taken care of first.

Please take a look and provide guidance if you will, I would like to see and make this happen, and will also like to help out in the meantime.

acald-creator avatar Jan 09 '24 21:01 acald-creator

@micahchiang it looks like you shared a confluence link. That is not open to everyone (just open to Platform Crew). So if you'd like to share that documentation, you'll need to do it in a different way.

andreahewitt-odd avatar Jan 10 '24 01:01 andreahewitt-odd

@andreahewitt-odd - ah thanks for the reminder. Sorry about that!

micahchiang avatar Jan 10 '24 16:01 micahchiang

@acald-creator - the initiatives the design system team are working on currently are to sunset Formation in favor of css-library, which will use USWDS v3 as one of its dependencies.

micahchiang avatar Jan 10 '24 16:01 micahchiang

@acald-creator - the initiatives the design system team are working on currently are to sunset Formation in favor of css-library, which will use USWDS v3 as one of its dependencies.

@micahchiang Thanks for bringing that up. I did see and acknowledge the css-library efforts.

Since I am not able to access the Confluence page, do you have a timetable on sunsetting Formation for css-library?

acald-creator avatar Jan 10 '24 16:01 acald-creator

This PR is stale because it has been open 120 days with no activity. It will recieve a stale label every day for 14 days before being closed unless it recieves a comment or the stale label is removed.

github-actions[bot] avatar May 12 '24 00:05 github-actions[bot]

This PR is stale because it has been open 120 days with no activity. It will recieve a stale label every day for 14 days before being closed unless it recieves a comment or the stale label is removed.

github-actions[bot] avatar Sep 26 '24 00:09 github-actions[bot]

awaiting PR to upgrade to node engine 20.9.0

  • https://github.com/department-of-veterans-affairs/vets-website/pull/31931

acald-creator avatar Sep 26 '24 14:09 acald-creator