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.

Sign up for GitHub

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

Jump to bottom

add script to header to load javascript runtime #1841

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

add script to header to load javascript runtime #1841

wants to merge 5 commits into from

Conversation

it-harrison
Copy link
Contributor

@it-harrison it-harrison commented Dec 21, 2023 •
edited by acrollet
Loading

Summary

This is a companion PR to this vets-website PR, which updates the vets-website webpack config to reduce bundle size. Part of this webpack change results in the extraction of the webpack runtime from each bundle. To make the bundles work in va.gov, therefore, a new script tag must be added to the <head> tag where the runtime is downloaded.

Related issue(s)

department-of-veterans-affairs/vets-website#27559

Testing done

Testing doc is here.

Screenshots

There should be no visual change due to this change.

Before:
Screenshot 2024-01-03 at 3 15 56 PM

After:

Screenshot 2024-01-03 at 3 17 19 PM

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

⚠️ Team Sites (only applies to modifications made to the VA.gov header) ⚠️

  • The 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?

@it-harrison it-harrison marked this pull request as ready for review January 3, 2024 21:19
@it-harrison it-harrison requested review from a team as code owners January 3, 2024 21:19
@jilladams
Copy link
Contributor

Can you confirm that you've verified whether there's any impact to the injected header?

How to test: https://github.com/department-of-veterans-affairs/vets-website/tree/main/src/applications/proxy-rewrite

rmessina1010
rmessina1010 approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@rmessina1010 rmessina1010 left a comment

Choose a reason for hiding this comment

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

LGTM

@va-vfs-bot va-vfs-bot temporarily deployed to master/main/add-script-for-runtime-js January 4, 2024 07:21 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/add-script-for-runtime-js January 4, 2024 12:42 Inactive
@it-harrison it-harrison mentioned this pull request Jan 10, 2024
Closed
randimays
randimays approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

LGTM!

This was referenced Jan 18, 2024
Closed
Closed
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/add-script-for-runtime-js March 15, 2024 05:32 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to master/main/add-script-for-runtime-js March 15, 2024 15:28 Inactive
@github-actions GitHub Actions
Copy link

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

@github-actions github-actions bot added the Stale label Jul 14, 2024
@github-actions github-actions bot closed this Jul 28, 2024
@acrollet acrollet reopened this Aug 8, 2024
@github-actions github-actions bot removed the Stale label Aug 9, 2024
@acrollet
Copy link
Contributor

acrollet commented Aug 9, 2024

Review instance with this branch + vets-website #27559 is available here: http://b7ae1dc6cd3e30dd6b86a7ed15ceb26a.review.vetsgov-internal/

The build completed successfully:

vets-website_1  | Step 53 of 55 start: Update robots.txt
vets-website_1  | Step 53 end [2ms]: Update robots.txt
vets-website_1  |
vets-website_1  | Step 54 of 55 start: Parse a virtual DOM from every .html file and perform a variety of DOM sub-operations on each file
vets-website_1  | 149755 broken links were found.
vets-website_1  | See report in /content-build/logs/localhost-broken-links.json.
vets-website_1  | Force Correct Accordion Level: 9:17.248 (m:ss.mmm)
vets-website_1  |
vets-website_1  | Parsed and created DOM for 29954 files.
vets-website_1  | Step 54 end [557290ms]: Parse a virtual DOM from every .html file and perform a variety of DOM sub-operations on each file
vets-website_1  |
vets-website_1  | Step 55 of 55 start: Ignore assets for build
vets-website_1  | Step 55 end [388ms]: Ignore assets for build
vets-website_1  | The Metalsmith build has completed.
vets-website_1  |
vets-website_1  | Adding debug info to Drupal pages...
vets-website_1  |
vets-website_1  | yarn run v1.19.1
vets-website_1  | $ webpack --config config/webpack.config.js --env=buildtype=localhost
vets-website_1  | Browserslist: caniuse-lite is outdated. Please run:
vets-website_1  |   npx browserslist@latest --update-db
vets-website_1  |   Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating
vets-website_1  | asset content-build.css 4.39 KiB [emitted] (name: content-build)
vets-website_1  | asset content-build.js 600 bytes [emitted] [minimized] (name: content-build)
vets-website_1  | Entrypoint content-build 4.98 KiB = content-build.css 4.39 KiB content-build.js 600 bytes
vets-website_1  | ./src/site/assets/sass/style.scss 50 bytes [built] [code generated]
vets-website_1  | css ./node_modules/css-loader/dist/cjs.js!./node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[0].use[2]!./node_modules/sass-loader/dist/cjs.js!./src/site/assets/sass/style.scss 4.39 KiB [built] [code generated]
vets-website_1  | webpack 5.90.3 compiled successfully in 3221 ms
vets-website_1  | Done in 7.19s.
vets-website_1  | Finished adding debug info to Drupal pages.
vets-website_1  |
vets-website_1  |
vets-website_1  | > [email protected] heroku-serve /content-build
vets-website_1  | > http-server "build/localhost" "-p" "3002"
vets-website_1  |
vets-website_1  | Starting up http-server, serving build/localhost
vets-website_1  | Available on:
vets-website_1  |   http://127.0.0.1:3002
vets-website_1  |   http://172.18.0.2:3002
vets-website_1  | Hit CTRL-C to stop the server

The runtime file is available here: http://b7ae1dc6cd3e30dd6b86a7ed15ceb26a.review.vetsgov-internal/generated/runtime.entry.js

The header, megamenu, footer and various applications all load and work correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmessina1010 rmessina1010 rmessina1010 approved these changes

@CBonade CBonade CBonade approved these changes

@randimays randimays randimays approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@it-harrison @jilladams @acrollet @randimays @CBonade @rmessina1010 @va-vfs-bot