Closed issackjohn closed 7 months ago
GIven all the changes, I'm wondering that wemay need to regenerate all dists as well. What do you think?
GIven all the changes, I'm wondering that wemay need to regenerate all dists as well. What do you think?
I had that as a todo item ... not sure if @issackjohn wants to run the builds for these anyways and I can do the same with the few that weren't touched afterwards?
I think it would be good to do that here to make sure that they still build after the changes.
I think it would be good to do that here to make sure that they still build after the changes.
Rebuild all the dists which had package-lock.json
updated in this PR right?
GIven all the changes, I'm wondering that wemay need to regenerate all dists as well. What do you think?
I had that as a todo item ... not sure if @issackjohn wants to run the builds for these anyways and I can do the same with the few that weren't touched afterwards?
Looks like news-next
doesn't work on github pages. @flashdesignory Would you be able to help me collect the before and after numbers for that? I can do the others.
5 runs
Other view: Before:
After:
I notice that jQuery (both simple and complex versions) shows quite a big increase on all browsers, do we know why?
It could come from the handlebars update. We were previously using a very old version from 2017.
This affects performance numbers a lot, so we should know more why that is happening. I don't think we should land this before there has been some more investigation on that.
I would like to scope this. The intentions of my change were to update the follow-redirects
package version to conform with internal compliance. Perhaps we could upgrade the other packages of jQuery in a separate PR? That way we don't block this one.
I would like to scope this. The intentions of my change were to update the follow-redirects package version to conform with internal compliance. Perhaps we could upgrade the other packages of jQuery in a separate PR? That way we don't block this one.
That seems fine to me - let's get this one closed with the development environment specific updates
It could come from the handlebars update. We were previously using a very old version from 2017.
This affects performance numbers a lot, so we should know more why that is happening. I don't think we should land this before there has been some more investigation on that.
I agree. We intentionally decided on framework versions for 3.0, and changing them should be deliberately considered - not based on the output of the audit command. We can consider whether updating this (along with other frameworks) is a good idea in a future version, there's plenty else to do now.
Thanks for the update Issack. Looking at the new diff, the following dist/ directories appear to have nontrivial changes:
Do we know if these are latent differences on main between what's in dist/
and the output of npm run build
, or are these changes as a result of something in the audit fix? Because presumably updating follow-redirects
should result in no changes to any dist.
Before
After:
Thanks for the update Issack. Looking at the new diff, the following dist/ directories appear to have nontrivial changes:
- newssite-next
- react-complex
- react-redux-complex
- react-redux
- react
- vue-complex
- vue
Do we know if these are latent differences on main between what's in
dist/
and the output ofnpm run build
, or are these changes as a result of something in the audit fix? Because presumably updatingfollow-redirects
should result in no changes to any dist.
The result of npm audit
and then rebuilding the dists. You're saying that we should only run npm update follow-redirects
in each of the affected packages of #351 right?
Besides jQuery with handlebars, I think that the other changes are fairly trivial. The large amount of upgraded packages comes mostly from babel. Have you seen different results in the packages you mentioned @bgrins ?
The result of npm audit and then rebuilding the dists. You're saying that we should only run npm update follow-redirects in each of the affected packages of https://github.com/WebKit/Speedometer/issues/351 right?
My suggestion is that we do two separate steps: first we just go through the existing directories and do npm run build
to start with a baseline of the current tree with no package.json changes - this is what #346 was opened for.
Then separately, we would do some kind of package.json updates (either the approach here or the approach in #351 - I don't have a strong opinion about this and haven't closely been following the discussion, but presumably the current approach in this PR is good), and do npm run build
again. That way we'll know exactly what is causing changes to the dist directory.
Besides jQuery with handlebars, I think that the other changes are fairly trivial. The large amount of upgraded packages comes mostly from babel. Have you seen different results in the packages you mentioned @bgrins ?
It's hard for me to tell what the substance of the changes are (it's hard to tell from the PR diff, and why I think it would be helpful to "clear out" any latent differences with main dist/ directories and the result of the build). But I'll defer to you on the review here: if you and others are satisfied with the current diff it's fine with me
GitHub has a nice viewer for the package-lock.json, you can click the button with the "document" icon. I believe we can revert the update of handlebars in jQuery. I'll look closely at the others on Monday, see if there's anything other than devDependencies.
Thanks for changing the jquery workload so that just "follow-redirects" is updated there.
I looked at all other updates, they're all dev dependencies: mostly babel, webpack, and related packages such as postcss. The angular package got some updates for angular-related packages too, but only build-related, not runtime.
Can you please provide new numbers in the various browsers so that we can see if the regression is still present? Thanks!
Before
After:
@julienw these are the new numbers after the commit that only changed jQuery
Before:
After:
OK, this looks good to me! Thanks for all the updates
OK, this looks good to me! Thanks for all the updates
Thanks!
@rniwa PTAL
Thank you all for your reviews and help!
This PR updates the package-lock.json files for the affected packages by running
npm audit fix
to address CVE-2023-26159.follow-redirects
to 1.15.5 instead of 1.15.4 as mentioned in a comment the issue.package-lock.json
was touched.Hosted at: https://issackjohn.github.io/Speedometer3
closes #355