Skip to content

Conversation

wesleybl
Copy link
Member

Backport of #7359 to Volto 18.

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

I have not done a detailed analysis to see what breaking changes happened between immer 8 and immer 10, and whether they will have an impact for Volto's dependencies that use this library. I was okay with making this update in Volto 19, since it's still in alpha and we have a chance to see whether the major version update causes any problems. But I don't know if it is safe to backport.

@wesleybl
Copy link
Member Author

@davisagli version 8 is used only as a development dependency. I tested with version 10 and had no issues. The development server worked without issue. Version 10 is already used in Volto, as it's a Slate dependency:

https://github.com/plone/volto/blob/18.x.x/pnpm-lock.yaml#L34149-L34151

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Okay. I think we do have a policy that we can make major updates to development dependencies. Worst case if we discover some problem later we can revert the change.

@sneridagh
Copy link
Member

@wesleybl @davisagli TBH, I would stop pushing for this upgrades to dependencies for the stable version. In Ferrara we learned that upgrading query-string to version 9 was not that innocuous. I am tempted to revert it. TL;DR: at some moment Node.js started to be more resilient to use ESM packages (query-string) in non-ESM builds (Razzle), but only from a certain version of Node.js 20. Remember @nileshgulia1 complaining about it? He showed me in Ferrara his setup, he was using an outdated Node 20 and the Volto build was failing. So we introduced an unexpected breaking change in there, since we claim we support (any) 20 and 22 in Volto 18.

Introducing new upgrades only increases the chance that this happens. If we are doing it, we have to be very sure that it won't break for anybody (which you never can assure 100%), study carefuly all the major versions that we are jumping, and then make an educated decision. Also, how do you know that the dependee of immer supports that version? Maybe they changed an API, and all of a sudden in somewhere else, it breaks.

@wesleybl what's the struggle with the old immer in your builds? what triggers this need? Can't you simply use the latest version in your add-on package and get along with it?

Regarding the query-string thing. Either we post a notice somewhere, or we revert the change. What do you think?

@wesleybl
Copy link
Member Author

@sneridagh Regarding the query-string, I gave up on updating it in Volto 18. See:

#7288 (comment)

But it was never in Volto 18. It's still a PR. So, there's nothing to revert. I even removed the update from the PR: d673f4e

Regarding immer, what motivated me to update it was that we performed a dependency-track vulnerability analysis (https://github.com/DependencyTrack/dependency-track) on a Volto project, and immer 8 was identified as having critical security vulnerabilities:

https://security.snyk.io/package/npm/immer

immer 8 is a development dependency of razzle. So I consider the chances of problems to be low. I tested the development server and the build on a large project and had no problems.Version 10 is already used in Volto, as it's a Slate dependency:

https://github.com/plone/volto/blob/18.x.x/pnpm-lock.yaml#L34149-L34151

Since it's to increase security, I considered updating it (and would like to do the same with other packages with critical vulnerabilities that don't have compatibility issues).

I can do this only on my project, but I don't think it's a good practice. If everyone does things in their own project, Volto won't evolve and it will be more difficult to update Volto on my project.

If anything goes wrong, we can undo it. But I respect whatever the decision is.

@wesleybl
Copy link
Member Author

@sneridagh Regarding query-string, the one forcing version 9 on branch 18 is @plone/client:

https://github.com/plone/volto/blob/18.x.x/packages/client/package.json#L62

There, I'm only in favor of reverting version 9 and using version 7. It seems that @plone/client is a "legacy alpha" on the main and 18.x.x branches. In its README, we have the warning:

Warning

Active development of this package is now on the seven branch. This package will be maintained as an alpha version only (1.0.0-alpha.x) on this branch (currently main, then when a branch is cut for 19.x.x) with bug fixes only. See breaking changes in the upcoming alphas.

See: https://github.com/plone/volto/blob/main/packages/client/README.md?plain=1#L17

This would also resolve issue plone/cookieplone-templates#236

I'll address this in #7288

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