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 and npm deps #485

Merged

Conversation

teknikal-wizard
Copy link
Collaborator

This updates all three webpack files (standard, test and minimal) to Webpack 5 format. It updates the npm dependencies to match. I also found that I had to pass the config explicitly to the fable command in the Run target.

Note - when testing I found that the tests are currently broken due to #465. I have submitted a fix for this as a separate PR, #484

@isaacabraham
Copy link
Member

Closes #472

@isaacabraham
Copy link
Member

cc: @Dzoukr would you be able to have a look at this config given your recent work?

@teknikal-wizard
Copy link
Collaborator Author

Fwiw I took the SAFEr template as my guide so should look pretty familiar.

I think we might be able to remove the hot: true from the devServer options as I spotted a hint saying it is the default anyway.

The only thing that stumped me for a few minutes was that the webpack config was no longer picked up automatically, but if I passed it in in the same way we do to for the tests then it worked fine.

@teknikal-wizard
Copy link
Collaborator Author

Also, I did spot npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it! in the Travis build error, so I guess that the CI machine might need an npm update?

@Dzoukr
Copy link

Dzoukr commented Nov 26, 2021

cc: @Dzoukr would you be able to have a look at this config given your recent work?

Looks good to me, but be aware that I am far from being good at this. I rather randomly change things and see if it works 😄

@teknikal-wizard
Copy link
Collaborator Author

Hi @theimowski, earlier today @isaacabraham asked me to see if you could take a look at the failing pipelines here? As I mention above, perhaps the Travis machine needs an npm update? If there is anything I can do to help just shout :) Cheers!

@theimowski
Copy link
Member

The Azure Pipelines seem to be failing due to different wording in new version of webpack. The tests look for a specified phrase in stdout which seems to be now webpack 5.64.2 compiled successfully in 2469 ms instead of ... : Compiled successfully. ...
Not sure yet about Travis failures, but could be as you say - due to incompatible node / npm versions - you can try fiddling with these lines

@theimowski theimowski mentioned this pull request Nov 30, 2021
@teknikal-wizard
Copy link
Collaborator Author

teknikal-wizard commented Dec 1, 2021

Just updated the stdOut phrase in the tests so hopefully that will sort Devops out. Also just found this thread which suggests the Travis failure is a node issue as I thought, so I will see if I can get that updated too.

@teknikal-wizard
Copy link
Collaborator Author

Node version seems update seemed to work, now both pipelines are failing on webpack being passed an unrecognised '-p' option. I'll see if I can fix that too.

@@ -80,7 +86,7 @@ module.exports = {
plugins: isProduction ?
commonPlugins.concat([
new MiniCssExtractPlugin({ filename: 'style.[name].[hash].css' }),
new CopyWebpackPlugin({ patterns: [{ from: resolve(CONFIG.assetsDir) }]}),
new CopyWebpackPlugin({ patterns: [{ from: resolve(CONFIG.assetsDir) }] }),
])
: commonPlugins.concat([
new webpack.HotModuleReplacementPlugin(),

Choose a reason for hiding this comment

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

Not completely necessary but since we already have hot: true in the devServer this plugin will be added by default, and we could remove it (check the tip inside the docs https://webpack.js.org/configuration/dev-server/#devserverhot)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Ryan Palmer added 3 commits December 1, 2021 12:46
… to the docs it was a shortcut for '--optimize-minimize --define process.env.NODE_ENV="production"'. We already set the node env in our webpack file, and webpack minimises prod code by default since v4.
@teknikal-wizard
Copy link
Collaborator Author

Ok @isaacabraham, all checks seem to be passing now

Copy link
Member

@theimowski theimowski left a comment

Choose a reason for hiding this comment

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

Seems dev server proxy stopped working - see comments

Content/default/webpack.config.js Show resolved Hide resolved
host: '0.0.0.0',
port: 8080,
inline: true,
proxy: {
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting following error when testing minimal template:

<e> [webpack-dev-server] [HPM] Error occurred while proxying request localhost:8080/api/hello to http://localhost:8085/ [ECONNREFUSED] (https://nodejs.org/api/errors.html#errors_common_system_errors)

maybe dev server proxy configuration changed format with new WebPack version?

Content/default/webpack.config.js Show resolved Hide resolved
Copy link
Member

@theimowski theimowski left a comment

Choose a reason for hiding this comment

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

The issue I referred to before was related to node v17

@theimowski theimowski merged commit da566b8 into SAFE-Stack:master Dec 7, 2021
@theimowski theimowski mentioned this pull request Dec 7, 2021
@isaacabraham isaacabraham deleted the upgrade_webpack_and_npm_deps branch January 21, 2022 11:49
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.

6 participants