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

Fix(dockerfile) Upgrade webpack version #522

Closed
wants to merge 1 commit into from

Conversation

smallteeths
Copy link
Contributor

@smallteeths smallteeths commented Jul 25, 2022

Signed-off-by: Siye Wang [email protected]

longhorn/longhorn#4283

@smallteeths smallteeths changed the title [WIP] fix(dockerfile) Upgrade npm version Fix(dockerfile) Upgrade npm version Jul 27, 2022
@smallteeths smallteeths changed the title Fix(dockerfile) Upgrade npm version Fix(dockerfile) Upgrade webpack version Jul 27, 2022
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

I saw many dependencies updated except webpack? I would suggest if they are not mandatory, don't update for now, because this PR will be backported to 1.3.1.

But we can do it after 1.3.1 is released. WDYT?

@smallteeths
Copy link
Contributor Author

I saw many dependencies updated except webpack? I would suggest if they are not mandatory, don't update for now, because this PR will be backported to 1.3.1.

But we can do it after 1.3.1 is released. WDYT?

https://webpack.js.org/migrate/5/#upgrade-webpack-to-5 According to the official documentation these dependencies need to be updated together with webpack. So upgrading webpack alone will not work. The dependency packages I updated this time were all used in the compile stage. It shouldn't affect the functionality of the page.

If we want to keep the updates to a minimum this time. This pr can be left alone for a while. I'll look into the current dependency package to fix some of the dependency versions. This may take some time.

@innobead
Copy link
Member

I saw many dependencies updated except webpack? I would suggest if they are not mandatory, don't update for now, because this PR will be backported to 1.3.1.
But we can do it after 1.3.1 is released. WDYT?

webpack.js.org/migrate/5/#upgrade-webpack-to-5 According to the official documentation these dependencies need to be updated together with webpack. So upgrading webpack alone will not work. The dependency packages I updated this time were all used in the compile stage. It shouldn't affect the functionality of the page.

If we want to keep the updates to a minimum this time. This pr can be left alone for a while. I'll look into the current dependency package to fix some of the dependency versions. This may take some time.

Thanks. If it will not impact functionality, then I am good for that. Let's go with the current approach of this PR.

@smallteeths
Copy link
Contributor Author

I saw many dependencies updated except webpack? I would suggest if they are not mandatory, don't update for now, because this PR will be backported to 1.3.1.
But we can do it after 1.3.1 is released. WDYT?

webpack.js.org/migrate/5/#upgrade-webpack-to-5 According to the official documentation these dependencies need to be updated together with webpack. So upgrading webpack alone will not work. The dependency packages I updated this time were all used in the compile stage. It shouldn't affect the functionality of the page.
If we want to keep the updates to a minimum this time. This pr can be left alone for a while. I'll look into the current dependency package to fix some of the dependency versions. This may take some time.

Thanks. If it will not impact functionality, then I am good for that. Let's go with the current approach of this PR.

fb55/htmlparser2#1237 (comment) This issue like ours. I researched the compilation issue we encountered this time, mainly due to the use of ^version in the longhorn dependencies. (^version “Compatible with version”, will update you to all future minor/patch versions, without incrementing the major version. ^2.3.4 will use releases from 2.3.4 to <3.0.0.).The Longhorn dependencies should have been recently updated and use the new syntax export * as ns. And this syntax is only available in webpack5. So upgrading webpack would be a good option.

@innobead
Copy link
Member

Sounds good to me.

@innobead
Copy link
Member

innobead commented Jul 28, 2022

As discussed with @smallteeths , even though this is just for the devDependencies update, we still need to minimize the change if possible for the patch release.

I would suggest the below items.

  1. Update the exact version match for @webassemblyjs/ast and @webassemblyjs/wasm-edit to 1.11.1, the previous successful version for all "@webassemblyjs" packages.

Because the current dependency strategy is "@webassemblyjs/ast": "^1.3.1", it will make the version upgrade to the latest minor release. At the moment, it's 1.11.3, but it uses the exact version dep of @webassemblyjs/helper-numbers 1.11.3 which does not exist, so the dep resolving failed when running npm install.

package.json
{
  "name": "longhorn",
  "version": "0.0.1",
  "private": true,
  "scripts": {
    "dev": "webpack-dev-server --config=webpack.config.development.js --mode development",
    "build": "cross-env NODE_ENV=production webpack --config=webpack.config.production.js --mode production",
    "lint": "eslint --ext .js src",
    "precommit": "npm run lint",
    "test": "jest"
  },
  "dependencies": {
    "@babel/runtime-corejs2": "^7.0.0",
    "antd": "3.26.16",
    "axios": "^0.21.1",
    "classnames": "^2.2.5",
    "cron-parser": "^2.11.0",
    "dva": "^2.4.1",
    "dva-loading": "^1.0.0",
    "history": "^4.7.2",
    "jquery": "^3.4.0",
    "meteor-later": "^1.2.0",
    "moment": "^2.29.2",
    "nprogress": "0.2.0",
    "prettycron": "^0.10.0",
    "prop-types": "^15.7.2",
    "qs": "^6.3.0",
    "query-string": "^6.4.2",
    "react": "16.8.4",
    "react-copy-to-clipboard": "^5.0.1",
    "react-countup": "4.1.3",
    "react-dom": "16.8.4",
    "react-helmet": "^5.0.0",
    "react-markdown": "^v4.3.1",
    "react-resize-detector": "1.1.0",
    "recharts": "^1.1.0",
    "robust-websocket": "^1.0.0",
    "semver": "^5.6.0"
  },
  "devDependencies": {
    "@babel/core": "^7.0.0",
    "@babel/plugin-proposal-class-properties": "^7.0.0",
    "@babel/plugin-proposal-decorators": "^7.0.0",
    "@babel/plugin-proposal-do-expressions": "^7.0.0",
    "@babel/plugin-proposal-export-default-from": "^7.0.0",
    "@babel/plugin-proposal-export-namespace-from": "^7.0.0",
    "@babel/plugin-proposal-function-bind": "^7.0.0",
    "@babel/plugin-proposal-function-sent": "^7.0.0",
    "@babel/plugin-proposal-json-strings": "^7.0.0",
    "@babel/plugin-proposal-logical-assignment-operators": "^7.0.0",
    "@babel/plugin-proposal-nullish-coalescing-operator": "^7.0.0",
    "@babel/plugin-proposal-numeric-separator": "^7.0.0",
    "@babel/plugin-proposal-optional-chaining": "^7.0.0",
    "@babel/plugin-proposal-pipeline-operator": "^7.0.0",
    "@babel/plugin-proposal-throw-expressions": "^7.0.0",
    "@babel/plugin-syntax-dynamic-import": "^7.0.0",
    "@babel/plugin-syntax-import-meta": "^7.0.0",
    "@babel/plugin-transform-runtime": "^7.0.0",
    "@babel/preset-env": "^7.0.0",
    "@babel/preset-react": "^7.0.0",
    "@webassemblyjs/ast": "1.11.1",
    "@webassemblyjs/wasm-edit": "1.11.1",
    "acorn": "^6.1.1",
    "add-asset-html-webpack-plugin": "^3.1.3",
    "address": "^1.0.3",
    "babel-eslint": "^8.2.3",
    "babel-loader": "^8.0.0",
    "babel-plugin-dva-hmr": "^0.4.1",
    "babel-plugin-import": "^1.7.0",
    "babel-plugin-transform-remove-console": "^6.9.2",
    "body-parser": "^1.18.3",
    "clean-webpack-plugin": "^0.1.19",
    "copy-webpack-plugin": "^4.5.1",
    "cross-env": "^5.1.1",
    "cross-port-killer": "^1.0.1",
    "css-hot-loader": "^1.4.4",
    "css-loader": "^2.1.1",
    "cssnano": "^4.1.10",
    "es5-imcompatible-versions": "^0.1.34",
    "eslint": "^5.16.0",
    "eslint-config-airbnb": "^17.1.0",
    "eslint-plugin-compat": "^2.2.0",
    "eslint-plugin-import": "^2.17.3",
    "eslint-plugin-jsx-a11y": "^6.0.3",
    "eslint-plugin-react": "^7.7.0",
    "estraverse": "^4.2.0",
    "expect": "^1.20.2",
    "file-loader": "^1.1.11",
    "friendly-errors-webpack-plugin": "^1.7.0",
    "happypack": "^5.0.0-beta.4",
    "html-webpack-plugin": "^3.2.0",
    "husky": "^1.0.0-rc.4",
    "jest": "^26.1.0",
    "keymaster": "^1.6.2",
    "less": "3.0.4",
    "less-loader": "^4.1.0",
    "mini-css-extract-plugin": "^0.4.0",
    "mockjs": "^1.0.1-beta3",
    "open-browser-webpack-plugin": "0.0.5",
    "optimize-css-assets-webpack-plugin": "^5.0.1",
    "pre-commit": "^1.2.2",
    "pro-download": "^1.0.1",
    "progress-bar-webpack-plugin": "^1.12.1",
    "redbox-react": "^1.5.0",
    "redux-devtools": "^3.4.1",
    "redux-devtools-dock-monitor": "^1.1.3",
    "redux-devtools-log-monitor": "^1.4.0",
    "regenerator-runtime": "^0.11.1",
    "sass-loader": "^7.0.1",
    "serve-index": "^1.9.1",
    "string-replace-loader": "^2.1.1",
    "style-loader": "^0.21.0",
    "type-is": "^1.6.15",
    "uglifyjs-webpack-plugin": "^1.2.5",
    "url-loader": "^1.0.1",
    "webpack": "^4.33.0",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.4",
    "webpack-manifest-plugin": "^2.0.4"
  },
  "overrides": {
    "htmlparser2": "^7.0.0"
  },
  "pre-commit": [
    "precommit"
  ]
}
  1. Override htmlparser2 back to the previous working version ^7.0.0 to ensure the compatibility with webpack 4

After resolving the first issue, the following comes with some updated dependencies (ex: html-to-react upgrading from 1.4 to 1.5) causing htmlparser2 to resolve to 8.0.x which will cause the current version of webpack 4.x unable to parse some syntax correctly as @smallteeths mentioned.

  1. Use package-lock.json instead of just relying on dynamic version update at runtime

Just use package-lock.json and don't need to try to figure out other versions in package.json, but this should be stabilized in 1.4.0 instead.

@smallteeths
Copy link
Contributor Author

Take David's suggestion to fix this issue by locking only the dependencies. refer to pr #526. cc @innobead

Close this for now.

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.

2 participants