Skip to content

Bump frontend dependencies#886

Merged
erxclau merged 3 commits intomasterfrom
npm-dep-bump
Mar 9, 2022
Merged

Bump frontend dependencies#886
erxclau merged 3 commits intomasterfrom
npm-dep-bump

Conversation

@erxclau
Copy link
Copy Markdown
Contributor

@erxclau erxclau commented Mar 7, 2022

What's this PR do?

  1. Bump vue-js-modal from 1.3.31 to 2.0.1

    • Breaking changes from version 2 do not affect us.
    • Used on account page profile settings to confirm if a user wants to leave the page with unsaved changes.
  2. Bump dompurify from 2.0.17 to 2.3.6

    • There are no breaking changing between these two releases that affect us.
    • Used to sanitize our query parameters to fill out the donation form fields automatically.
  3. Bump webpack-cli from 3.3.4 to 4.9.2

    • There are no breaking changes for us in bumping the CLI.
    • I believe we only use the CLI to pass in a config file when building our JS:
        "js:dev": "NODE_ENV=development webpack --config webpack/config.dev.js",
        "js:prod": "NODE_ENV=production webpack --config webpack/config.prod.js",
  4. Bump eslint from 5.3.0 to 7.32.0

    • Version 7.32.0 is sufficient for resolving a vulnerability and I did not want to upgrade to version 8 because it introduces more changes that need to be made.
    • Bump eslint-config-airbnb-base from 13.2.0 to 14.2.0 to support eslint@7.x as noted in the release notes.
      • Bumping from v13 to v14 introduces the max-classes-per-file rule as noted here on the third line. This requires that we only have one class in a file. There are several classes in static/js/src/errors.js and static/js/src/entry/account/errors.js so I added disabling comments in those two files.
    • Bump eslint-plugin-vue from 6.2.2 to 7.2.0 to support eslint@7.x as noted in the release notes.
      • The upgrade recommends replacing v-slot with the # shorthand. I resolved these errors with npx eslint --ext .js,.vue static/js/src config --fix.
      • The upgrade recommends using kebab-case instead of camelCase for custom event names. Given that our code works, I decided to just keep the camelCase names and add a rule in package.json to allow camelCase:
        "vue/custom-event-name-casing": [
                "error",
                "camelCase"
        ]

Why are we doing this? How does it help us?

  • Groups dependabot PRs together.
  • npm audit shows that bumping webpack-cli and eslint (and related packages) will resolve chalk/ansi-regex vulnerabilities. A ansi-regex Dependabot alert currently exists.

How should this be manually tested?

vue-js-modal

  • Change profile settings on the account page but do not click the save button.
  • Click on buttons on the page to attempt to navigate away from the page.
    • On production, clicking on Account Overview (in the side and top navigation bars) and Tribune Ambassadors will bring up the modal. Clicking on Donate or Log out will not bring up the modal. Closing the tab will not bring up the modal either.
  • In the modal, click on the Keep editing, Don't save, and close buttons to test for proper behavior.

dompurify

  • Add some query parameters to the URL such as firstName, lastName, email, and zipcode.
  • Note that the donation form will have portions automatically filled out based on these parameters.
  • The main purpose of dompurify is to sanitize HTML/SVG elements from input but by nature of a URL, you can't have / as a character because that'd be a different route entirely.

webpack-cli, eslint and related packages

  • npm install and make restart and npm run lint
  • Click around the app and make sure all the JS features seem to work and there are no new console warnings
  • Test the donations form
  • Confirm you can log into the portal

How should this change be communicated to end users?

N/A

Are there any smells or added technical debt to note?

No

What are the relevant tickets?

https://3.basecamp.com/3098728/buckets/736178/todos/4696911684

Have you done the following, if applicable:

(optional: add explanation between parentheses)

  • Added automated tests? ( )
  • Tested manually on mobile? ( )
  • Checked BrowserStack? ( )
  • Checked for performance implications? ( )
  • Checked accessibility? ( )
  • Checked for security implications? ( )
  • Updated the documentation/wiki? ( )

TODOs / next steps:

  • your TODO here

@x110dc x110dc temporarily deployed to donations-testing-pr-886 March 7, 2022 17:05 Inactive
@x110dc x110dc temporarily deployed to donations-testing-pr-886 March 8, 2022 16:44 Inactive
Comment on lines +67 to +85

data() {
return {
showModal: false,
checkModalResolve: () => {},
};
},

methods: {
setShowModal(shouldShow) {
this.showModal = shouldShow;
},

checkModalAction() {
return new Promise((resolve) => {
this.checkModalResolve = resolve;
});
},
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

eslint --fix just moved this code underneath beforeRouteLeave. Nothing about the code itself changed.

@erxclau erxclau marked this pull request as ready for review March 8, 2022 19:42
Copy link
Copy Markdown
Member

@ashley-hebler ashley-hebler left a comment

Choose a reason for hiding this comment

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

Another well documented PR! This was lovely, Eric.

I ran through all the steps in the various places and all WFM. I like those vue syntax changes — a little cleaner to read/scan. Agree with your call on staying on eslint 7 to minimize major changes. Awesome that we're even squashing a vulnerability in the process!

My test donation came through on slack so I think this should be fine to merge at your convenience.

@erxclau erxclau merged commit ec83b3d into master Mar 9, 2022
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.

3 participants