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

chore: update dependencies #2484

Merged
merged 2 commits into from Sep 13, 2021
Merged

chore: update dependencies #2484

merged 2 commits into from Sep 13, 2021

Conversation

nolanlawson
Copy link
Contributor

Details

Our regularly-scheduled dependency bump. yarn audit is now at 0 vulnerabilities. 🎉

Does this PR introduce breaking changes?

  • ✅ No, it does not introduce breaking changes.

nolanlawson
nolanlawson commented Sep 7, 2021
View reviewed changes
package.json
@@ -24,45 +24,46 @@
"release:publish:ci": "./scripts/release/publish.js",
"release:version": "./scripts/release/version.js"
},
"//": "Currently can't upgrade TypeScript to v4.4.2 because it breaks Rollup: https://git.io/JuCcs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"//" is a safe and encouraged way to add comments to package.json.

I felt this was appropriate because otherwise it may be confusing for the next person why we're stuck on TypeScript v4.3.5.

This will be fixed in TypeScript 4.4.3: rollup/plugins#983 (comment)

nolanlawson
nolanlawson commented Sep 7, 2021
View reviewed changes
packages/@lwc/template-compiler/package.json Outdated
"@types/he": "^1.1.1",
"@types/parse5": "^6.0.1"
},
"//": "Currently can't upgrade estree-walker to v3 because it breaks karma tests due to ESM: https://git.io/JuCcq",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to upgrade estree-walker to v3 we would need to get rid of our CommonJS build output for @lwc/template-compiler. Rich-Harris/estree-walker#26

nolanlawson
nolanlawson commented Sep 7, 2021
View reviewed changes
package.json Outdated
"node-fetch": "^2.6.1",
"systeminformation": "^5.3.1",
"xmlhttprequest-ssl": "^1.6.2"
"micromatch": "^4.0.4"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these resolutions were added to fix security vulnerabilities. The only one we still need is micromatch, which can be fixed when we update rollup-plugin-compat's dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking care of this

Copy link
Member

Choose a reason for hiding this comment

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

It should be fixed with: https://github.com/salesforce/es5-proxy-compat/commit/012991ede3f48fe7b651b07f0c41afe7012bfd2a.

As a side note, I don't see the point of using resolutions in this repo when it comes to NPM audit issues, except when it's a major security vulnerability (eg. eslint-scope). The resolutions property is only applied to this repository. It makes sense if we were shipping an application with tight dependency control, but in our case where we are shipping libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this very particular case, it actually does work because es5-proxy-compat is a dev dependency that we bundle into @lwc/engine-core. but I agree; resolutions kind of rubs me the wrong way, unless it's for a really critical security vulnerability, and if it actually affects consumers.

I'll publish a new es5-proxy-compat so we can get rid of resolutions entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all the deps and got rid of resolutions!

@ekashida
Copy link
Member

ekashida commented Sep 7, 2021

Nit: I think I might be the only one still following this convention but used to only add the package name in the parenthesis. I believe it was because we had just started experimenting with hand-written release notes and wanted to keep the description in such a format that we could easily go back to generated release notes.

@nolanlawson
Copy link
Contributor Author

OK, I can remove (package) since this applies to multiple packages.

@nolanlawson nolanlawson changed the title chore(package): update dependencies chore: update dependencies Sep 7, 2021
@nolanlawson nolanlawson mentioned this pull request Sep 7, 2021
Closed
@ekashida ekashida closed this Sep 7, 2021
@ekashida ekashida reopened this Sep 7, 2021
@nolanlawson nolanlawson mentioned this pull request Sep 10, 2021
Closed
@nolanlawson
Copy link
Contributor Author

Looks like a true failure, will investigate...

[chrome 93.0.4577.63 Windows #0-49] Running: chrome (v93.0.4577.63) on Windows
[chrome 93.0.4577.63 Windows #0-49] Session ID: d664c4313b6d482f8f48bbcc541912b6
[chrome 93.0.4577.63 Windows #0-49]
[chrome 93.0.4577.63 Windows #0-49] » /src/components/events/test-clipboard-event-composed/clipboard-event-composed.spec.js
[chrome 93.0.4577.63 Windows #0-49] clipboard-event-composed polyfill
[chrome 93.0.4577.63 Windows #0-49]    ✖ "before all" hook for clipboard-event-composed polyfill
[chrome 93.0.4577.63 Windows #0-49]
[chrome 93.0.4577.63 Windows #0-49] 1 failing (176ms)
[chrome 93.0.4577.63 Windows #0-49]
[chrome 93.0.4577.63 Windows #0-49] 1) clipboard-event-composed polyfill "before all" hook for clipboard-event-composed polyfill
[chrome 93.0.4577.63 Windows #0-49] Cannot read property 'finally' of undefined
[chrome 93.0.4577.63 Windows #0-49] TypeError: Cannot read property 'finally' of undefined
[chrome 93.0.4577.63 Windows #0-49]     at Context.executeAsync (/home/circleci/lwc/node_modules/@wdio/utils/build/shim.js:293:42)
[chrome 93.0.4577.63 Windows #0-49]     at Context.testFrameworkFnWrapper (/home/circleci/lwc/node_modules/@wdio/utils/build/test-framework/testFnWrapper.js:45:32)

@nolanlawson
Copy link
Contributor Author

Found what appears to be a bug in WebDriverIO: webdriverio/webdriverio#7405

We can work around it temporarily by using async functions in before() hooks though.

@nolanlawson nolanlawson merged commit c929f8f into master Sep 13, 2021
@nolanlawson nolanlawson deleted the nolan/update-deps-202109 branch September 13, 2021 16:36
@nolanlawson nolanlawson mentioned this pull request Oct 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekashida ekashida ekashida approved these changes

@pmdartus pmdartus pmdartus 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.

None yet
3 participants
@nolanlawson @ekashida @pmdartus