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.

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

Upgrade webpack to version 5 #6690

Merged
merged 8 commits into from
Dec 1, 2022

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Jul 10, 2022

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets parts of #6343
Related issues/PRs requires #6688, sulu/skeleton#201
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Upgrade webpack to version 5.

Why?

Support latest version of webpack.

TODO

  • Remove dependencies to util, process packages via isemail package.
  • Remove dependency to Buffer package see own js
  • Undo changes of 822516f
  • Fix Build with skeleton
  • Replace Email Validator

@alexander-schranz alexander-schranz force-pushed the feature/upgrade-webpack-5 branch 3 times, most recently from abc680a to 1538bb9 Compare July 10, 2022 16:32
@alexander-schranz alexander-schranz marked this pull request as draft July 10, 2022 17:52
@alexander-schranz
Copy link
Member Author

While it did first look good to build with webpack 5 there are issues with the isemail package. There are currently 3 node packages which we require util, process, Buffer. This is not longer supported. The workarounds via webpack and browserify didn't work in combination with skeleton. So we will currently keep on webpack 4 until resolving this. Issues.

@alexander-schranz alexander-schranz added the DX Affecting the end developer label Jul 10, 2022
@niklasnatter
Copy link
Contributor

I think I would be okay with replacing the isemail if this is a blocker in the future. Where did we use the Buffer class in out code? 🤔

"mobx": "^4.0.0",
"mobx-react": "^5.0.0",
"moment-timezone": "^0.5.14",
"optimize-css-assets-webpack-plugin": "^6.0.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

replaced by css-minimizer-webpack-plugin: https://github.com/NMFR/optimize-css-assets-webpack-plugin

"@wojtekmaj/enzyme-adapter-react-17": "^0.6.1",
"autoprefixer": "^9.8.6",
"babel-eslint": "^10.0.2",
"babel-jest": "^26.3.0",
"babel-loader": "^8.0.6",
"clean-webpack-plugin": "^4.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

think we not longer need this with clean: true

@@ -87,10 +86,9 @@
"stylelint": "^13.2.1",
"stylelint-config-standard": "^22.0.0",
"url-search-params-polyfill": "^8.0.0",
"webpack": "^4.27.0",
"webpack-clean-obsolete-chunks": "^0.4.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

The clean: true is the new way for this: https://github.com/GProst/webpack-clean-obsolete-chunks

Comment on lines -104 to -105
// set path to public root from bundled css: https://github.com/sulu/sulu/pull/6225
publicPath: path.relative(outputPath, '.'),
Copy link
Member Author

Choose a reason for hiding this comment

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

not longer required as all pathes in css/js are relative

Copy link
Contributor

Choose a reason for hiding this comment

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

this is nice 🥳 i think the reason we dont need it anymore is because we configure the MiniCssExtractPlugin with a relative filename option

@@ -4,8 +4,8 @@ import {observer} from 'mobx-react';
import {action, computed, observable} from 'mobx';
import classNames from 'classnames';
import log from 'loglevel';
import IsEmail from 'isemail';
Copy link
Member Author

Choose a reason for hiding this comment

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

Was never planned by the package to support browsers. They only support node based applications.

return false;
}

return /^(.+)@(.+)$/.test(value);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is very dump regex. Not sure how strict we should even implement things here. /cc @sulu/core-team

We definitely should not be too strict in case of supporting also local and intranet domains @localhost, @massive.local, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -89,14 +88,12 @@
"react-styleguidist": "^11.0.5",
"react-test-renderer": "^17.0.0",
"regenerator-runtime": "^0.13.3",
"style-loader": "^2.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this as it seems not longer be used by us, think the css extract is working differently for us. And in the skeleton it was not even installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it was used by react-styleguidist? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

okay i think this was a leftover from a previous ckeditor configuration that was copied from their documentation: https://ckeditor.com/docs/ckeditor5/latest/installation/advanced/alternative-setups/integrating-from-source.html

Copy link
Member Author

Choose a reason for hiding this comment

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

yes did see it in the ckeditor and I was thinking that I already once had that case. That I tried to use the style-loader but did then come to the conclution that it isn't compatible or require for our setup.

@alexander-schranz alexander-schranz marked this pull request as ready for review November 30, 2022 00:14
Comment on lines -8 to -11
test('Email address with emojis should pass validation', () => {
expect(idnEmailValidator('🤌@😎')).toBe(true);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Test was introduced by @luca-rath in #5815 but even browsers yet does not support this. So I think we can safely remove it:

Bildschirmfoto 2022-11-30 um 01 21 48

UPGRADE.md Outdated Show resolved Hide resolved
UPGRADE.md Outdated Show resolved Hide resolved
Co-authored-by: Niklas Natter <niklas.natter@gmail.com>
@@ -42,7 +42,7 @@

.selectMultipleButtonContainer {
display: flex;
justify-content: end;
Copy link
Contributor

Choose a reason for hiding this comment

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

was this change intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes autoprefixer did complain that flex-end is supported better then end and has same effect and did output a warning message about this line.

webpack.config.js Outdated Show resolved Hide resolved
publicPath,
clean: true,
path: path.resolve(projectRootPath, publicDir, outputPath),
filename: '[name].[chunkhash].js',
},
stats: 'minimal',
performance: {
hints: false,
},
devtool: argv.mode === 'development' ? 'eval-source-map' : 'source-map',
Copy link
Contributor

Choose a reason for hiding this comment

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

the build process works for me and the result looked good during my testing 🥳
unfortunately, it looks like our npm run watch task does not work anymore. right now, i see the changes only after restarting the watch task

Copy link
Contributor

Choose a reason for hiding this comment

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

the output of my console indicates that the change was detected, but no changes are written to the filesystem if i change the sulu source code

Copy link
Contributor

Choose a reason for hiding this comment

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

okay i suspect that this is somehow caused by the location of the code of Sulu 😢 if i change the code of the assets/admin/app.js file, the watch task works as expected. maybe files are not reloaded if they are located within the node_modules folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay it looks like we are running into this issue: webpack/webpack#11612
adding the following to the configuration fixes the behaviour of the watch task:

snapshot: {
    // detect changes in "node_modules/@sulu": https://github.com/webpack/webpack/issues/11612
    managedPaths: [], 
},

alexander-schranz and others added 2 commits December 1, 2022 08:26
Co-authored-by: Niklas Natter <niklas.natter@gmail.com>
Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

🥳

@niklasnatter niklasnatter merged commit 71e9312 into sulu:2.6 Dec 1, 2022
@alexander-schranz alexander-schranz deleted the feature/upgrade-webpack-5 branch December 1, 2022 08:42
@alexander-schranz
Copy link
Member Author

🎉

@alexander-schranz alexander-schranz added this to the Release 2.6 milestone Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Affecting the end developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants