-
-
Couldn't load subscription status.
- Fork 803
Add package @volto/razzle #7542
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.
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
base: main
Are you sure you want to change the base?
Conversation
It's a fork of Razzle. Razzle is no longer maintained. So with this fork, we can update versions of packages with vulnerabilities.
|
@davisagli @sneridagh in this case do we need to fix the lint or can we ignore it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case do we need to fix the lint or can we ignore it?
In my opinion we need to either apply the fixes, or change the lint configuration for this package so the existing code doesn't fail the checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set up towncrier like we have for other packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep the jest stuff, since Volto is now using vitest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I even tried removing this file, but the cookieplone job complained. Even though it uses Volto 19, it still uses Jest on Volto 19. But I don't see a problem with keeping this file here, so it looks like the original razzle.
Another thing: Can we have this change on Volto 18?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesleybl is using jest in cookieplone tests? we should address that (at least in 19). I will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesleybl @davisagli I guess we have to merge the 19 specifics branch in cookieplone, since it stills tries to use jest in ci-test. Let's try to make that happen soon. I am working in the cypress15 upgrade, some tests are still failing. I asked for helpt to Tisha, but all hands are welcomed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneridagh @davisagli I think we can merge the Volto 19 branch from cookieplone as is. What do you think?
We need to merge first: plone/cookieplone-templates#299
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the jest config anymore, but if we do, we should now just change it in our fork instead of applying a patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We first need to have vitest in cookieplone so that its job doesn't complain. After that, we can remove this patch and all Jest configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davisagli indeed, we need to move the patch to the right place in the fork, and remove the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneridagh In fact, after we removed Jest support, we didn't need to apply the patch anywhere.
@davisagli fixed. |
|
@davisagli Is this considered a breaking change? Could we have this in Volto 18? |
|
@wesleybl It's definitely a breaking change, since it requires updating babel.js and the storybook script. I wouldn't try to push it for Volto 18 unless something is actively broken with the existing razzle package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just formatting and MyST markup stuff. Otherwise LGTM! Let's make sure it builds correctly in the PR preview at https://volto--7542.org.readthedocs.build/7542/upgrade-guide/index.html#replace-razzle-with-volto-razzle-fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks already very good!
| ```diff | ||
| "devDependencies": { | ||
| - "razzle": "4.2.18", | ||
| + "@volto/razzle": "x.x.x", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be necessary in projects/add-ons? I don't think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about using the old Volto project generator. But I don't think we need to worry about that anymore. I'll remove it.
| 3. In most cases, you don't need to change your scripts—for example `razzle start`, `razzle build`, or `razzle test`—because the fork preserves the original CLI entrypoints. | ||
| If you have code that imports internal modules from the `razzle` package, for example, `require('razzle/some/path')`), then update those imports to reference `@volto/razzle` instead. | ||
|
|
||
| 4. Search your project for any direct or indirect references to `razzle` to ensure nothing was left behind, including imports, requires, and configuration presets or plugins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to mention, but I think that won't be necessary in 99% of cases. So I'd state that it's only for people that really tapped into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would have to be changed here:
So I think it's better to leave it.
| @@ -0,0 +1,69 @@ | |||
|  | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a disclaimer at the top, specifying it's a fork and the reason behind. Maintaining attibutions.
| "webpackbar": "~4||~5" | ||
| }, | ||
| "peerDependencies": { | ||
| "babel-preset-razzle": "4.2.18", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesleybl did you investigate if we will need to fork this one too? As long as I remember, this one contained all the babel deps, which will be terrible outdated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneridagh In this PR I'm only concerned about the razzle. But we'll definitely have to investigate this one too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davisagli indeed, we need to move the patch to the right place in the fork, and remove the patch.
It's a fork of Razzle. Razzle is no longer maintained. So with this fork, we can update versions of packages with vulnerabilities.
I used the master branch of
razzle📚 Documentation preview 📚: https://volto--7542.org.readthedocs.build/