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

USWDS-Compile - Dependencies: GulpJS v5 #96

Merged
merged 5 commits into from
Jul 15, 2024
Merged

USWDS-Compile - Dependencies: GulpJS v5 #96

merged 5 commits into from
Jul 15, 2024

Conversation

mejiaj
Copy link
Contributor

@mejiaj mejiaj commented Apr 5, 2024

Updating GulpJS from v4 to v5. Closes #99.

Gulp 5 changelog →

Before (develop)

14 vulnerabilities (3 moderate, 11 high)

After

3 moderate severity vulnerabilities

Previews

Testing

Possible regressions

Issues with assets and compile

Details

Possible regression

I tested updating Gulp to 5.0.0 in #101. When installing the branch on our Site and Sandbox repos, I saw a possible regression in Sandbox when trying to run the copyAssets and copyImages compile scripts. The images would become corrupt and not be able to open properly. For whatever reason, this was not an issue on our Site repo.

Source →.

Regressions in Sandbox with copyAssets and copyImages resulting in regressions and corrupted images.

1. Missing flag in Site

2. Missing icons in Sandbox

Wasn't able to reproduce. Tested with/without encoding in 424d9a4 and 294b40b.

3. Font rendering

Details

When I uswds/uswds-site#2719, I noticed that the fonts no longer rendered correctly (See screenshot below). When I tested reverting the branch to version ^1.1.0, the fonts rendered correctly again.

Source →

Confirmed in Site and Sandbox. Fonts were corrupted and not rendering correctly on site.

@mejiaj mejiaj marked this pull request as ready for review June 17, 2024 19:02
@mejiaj mejiaj added this to the compile 1.2.0 milestone Jun 17, 2024
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

  • Confirmed that I could successfully perform all the gulp tasks in the uswds-sandbox test branch
  • Confirmed that I could successfully perform gulp tasks after installing on uswds-site
    • When I installed this branch on uswds-site, I noticed that the fonts no longer rendered correctly (See screenshot below). When I tested reverting the branch to version ^1.1.0, the fonts rendered correctly again.
      image
    • Also, noting that to get the gulp watchSass task to run without error, I needed to remove gulp 4 from the package.json in the uswds-site repo.
  • Confirmed the vulnerabilities listed in the PR description are accurate and up-to-date

Follow-up tasks

The documentation on the Phase 2: Compile page specifically mentions to tool using Gulp 4. We should be sure to update that after this PR is merged.

@amyleadem amyleadem requested a review from mahoneycm June 27, 2024 15:40
@mahoneycm
Copy link
Contributor

Confirming that I'm seeing similar compilation errors in both Sandbox and Site surrounding image and font compilation.

Img compilation error

Banner missing American flag icon

Various img’s in assets folder are failing to load/compile:

  • circle-124.png
  • hero.jpg
  • hero.webp
  • logo-img
  • us_flag_small.png

Looks like all other nested images are compiling fine, just a few in the root assets folder are missing.

Font compilation error

Console errors

image

Installing Gulp5 on Site reduces the number of errors but I see the same results on the page.
image

Trying to open the fonts files in VS code gives the following error:

The file is not displayed in the text editor because it is either binary or uses an unsupported text encoding.

Addresses compression and loading issues in browsers.

Example browser errors:
- `Failed to decode downloaded font: <URL>`
- `OTS parsing error: Failed to convert WOFF 2.0 font to SFNT`
Resolves broken image issue in Cloud Page previews
Resolves 404 issues in Cloud Page previews
@@ -255,6 +261,7 @@ function buildSprite() {
function renameSprite() {
return src(`${paths.dist.img}/usa-icons.svg`.replace("//", "/"), {
allowEmpty: true,
encoding: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note

Didn't notice any difference here. Keeping because SVGs it still following recommendation for removing encoding from images.

@mejiaj
Copy link
Contributor Author

mejiaj commented Jul 1, 2024

I was able to confirm the following issues:

  • Fonts corrupt/not rendering
  • Images corrupt/not rendering
  • Icon regressions test page

Any help reproducing icon issue would be appreciated.

CC @amyleadem @mahoneycm

@mejiaj mejiaj requested a review from amyleadem July 1, 2024 21:42
Comment on lines +114 to +116
return src(`${getSrcFrom("fonts")}/**/**`.replace("//", "/"), {
encoding: false,
}).pipe(dest(paths.dist.fonts));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoids corruption on images and fonts.

More info here: gulpjs/gulp#2803

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! And thanks for the reference link 👏

Copy link
Contributor

@mahoneycm mahoneycm 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 looking great!

  • Previous Sandbox regressions resolved
  • Previous Site regressions resolved
  • Scripts run without error
  • Update resolves a high severity dependency vulnerability

Also noting that I never observed any icon regression that you mentioned being unable to reproduce. All the img regressions I saw were specifically jpg, png, or webp files specifically in the /assets/img/ directory which have since been resolved 👍

Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your work on this. 👏

In both uswds-site and uswds-sandbox test branches, I was able to confirm:

  • Fonts appear to render correctly
  • Images appear to render correctly
  • No console errors
  • Able to successfully run gulp tasks

Also:

  • Confirm return src() calls in gulpfile.js turn off encoding for images and fonts (thanks for the helpful note, @mejiaj!)

Comment on lines +114 to +116
return src(`${getSrcFrom("fonts")}/**/**`.replace("//", "/"), {
encoding: false,
}).pipe(dest(paths.dist.fonts));
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! And thanks for the reference link 👏

Copy link
Contributor

@heymatthenry heymatthenry left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@mejiaj mejiaj merged commit 2d7750a into develop Jul 15, 2024
1 check passed
@mejiaj mejiaj deleted the jm-gulp5 branch July 15, 2024 15:42
@mdmower-csnw
Copy link
Contributor

@mejiaj - Oooh, super important, critical, drop-everything-the-project-is-on-fire change still needs to be made along with this PR... the readme needs to change "Gulp 4" to "Gulp 5"! 😉

@angeliquejw
Copy link

Is there a timeline for when these changes will also be pushed to the package?

@annepetersen
Copy link

(I don't have the best info to answer your question, but: hi @angeliquejw! good to "see" you 👋)

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

Successfully merging this pull request may close these issues.

USWDS-Compile - Feature: Support gulp 5
7 participants