-
Notifications
You must be signed in to change notification settings - Fork 2
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 dependencies webpack craco #140
Changes from all commits
a4392f8
9a789ea
5c36620
5405c08
d7f6b65
ce8e746
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
const { addBeforeLoader, loaderByName, whenTest } = require("@craco/craco"); | ||
const { whenTest } = require("@craco/craco"); | ||
const SentryWebpackPlugin = require("@sentry/webpack-plugin"); | ||
// const webpack = require("webpack"); | ||
const webpack = require("webpack"); | ||
|
||
module.exports = { | ||
babel: { | ||
|
@@ -29,25 +29,18 @@ module.exports = { | |
}, | ||
webpack: { | ||
configure: (webpackConfig) => { | ||
const wasmExtensionRegExp = /\.wasm$/; | ||
webpackConfig.resolve.extensions.push(".wasm"); | ||
// Verbose output from Webpack to help debugging on CI | ||
if (process.env.CI) webpackConfig.stats = "verbose"; | ||
|
||
webpackConfig.module.rules.forEach((rule) => { | ||
(rule.oneOf || []).forEach((oneOf) => { | ||
if (oneOf.loader && oneOf.loader.indexOf("file-loader") >= 0) { | ||
oneOf.exclude.push(wasmExtensionRegExp); | ||
} | ||
}); | ||
webpackConfig.module.rules.push({ | ||
test: /\.wasm$/, | ||
type: "webassembly/async", | ||
}); | ||
|
||
const wasmLoader = { | ||
test: /\.wasm$/, | ||
include: /node_modules\/(bridge|token-bridge)/, | ||
loaders: ["wasm-loader"], | ||
webpackConfig.experiments = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, how much confidence should we have in this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not very familiar with wasm to be honest but from the documentation it seems they've implemented the latest specification so if it works during our tests I wouldn't worry too much. We can stay to a specific webpack version if problems occur in the future plus it's one less dependency. |
||
asyncWebAssembly: true, | ||
}; | ||
|
||
addBeforeLoader(webpackConfig, loaderByName("file-loader"), wasmLoader); | ||
|
||
// Disable code splitting to prevent ChunkLoadError | ||
webpackConfig.optimization.runtimeChunk = false; | ||
webpackConfig.optimization.splitChunks = { | ||
|
@@ -68,6 +61,27 @@ module.exports = { | |
? "static/js/[name].[hash].js" | ||
: "static/js/[name].[chunkhash].js"; | ||
|
||
// add polyfills that are not included in webpack 5 | ||
webpackConfig = { | ||
...webpackConfig, | ||
ignoreWarnings: [/Failed to parse source map/], | ||
resolve: { | ||
...webpackConfig.resolve, | ||
fallback: { | ||
fs: require.resolve("browserify-fs"), | ||
path: require.resolve("path-browserify"), | ||
stream: require.resolve("stream-browserify"), | ||
crypto: require.resolve("crypto-browserify"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, they were included by default in webpack v4 but removed in v5? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, all of them were added based on webpack's own output. They guide on adding the polyfills your bundle needs. |
||
}, | ||
}, | ||
}; | ||
|
||
webpackConfig.plugins.push( | ||
new webpack.ProvidePlugin({ | ||
Buffer: ["buffer", "Buffer"], | ||
}), | ||
); | ||
|
||
if (process.env.NODE_ENV !== "development") { | ||
webpackConfig.devtool = "source-map"; | ||
|
||
|
@@ -84,6 +98,7 @@ module.exports = { | |
release: process.env.SENTRY_RELEASE, | ||
include: "build", | ||
ignoreFile: ".gitignore", | ||
urlPrefix: "~/static/js", | ||
}), | ||
]; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,9 @@ | |
"private": true, | ||
"version": "0.1.0", | ||
"scripts": { | ||
"start": "NODE_ENV=development SKIP_PREFLIGHT_CHECK=true DISABLE_ESLINT_PLUGIN=true craco start", | ||
"build": "craco build", | ||
"release": "SENTRY_RELEASE=$(git rev-parse --short HEAD) SKIP_PREFLIGHT_CHECK=true DISABLE_ESLINT_PLUGIN=true craco build", | ||
"start": "NODE_ENV=development DISABLE_ESLINT_PLUGIN=true craco start", | ||
"build": "DISABLE_ESLINT_PLUGIN=true craco build", | ||
"release": "SENTRY_RELEASE=$(git rev-parse --short HEAD) DISABLE_ESLINT_PLUGIN=true craco build && rm build/static/js/*.map && echo 'deleted sourcemaps files due to cloudflare file limits https://developers.cloudflare.com/pages/platform/limits/#file-size'", | ||
"test": "craco test --transformIgnorePatterns \"node_modules/(?!@project-serum/sol-wallet-adapter)/\"", | ||
"eject": "react-scripts eject", | ||
"format": "prettier --write \"./src/**/*.{ts,tsx}\" \".storybook/**/*.{js,ts,tsx}\"", | ||
|
@@ -35,7 +35,7 @@ | |
}, | ||
"dependencies": { | ||
"@certusone/wormhole-sdk": "^0.2.6", | ||
"@craco/craco": "^6.3.0", | ||
"@craco/craco": "7.0.0-alpha.3", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks scary. I skimmed that thread on Slack - did we decide we should move off craco and if so is there a ticket for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely thought about that since we believed they won't be releasing a new version with I think that's fine since we can use this version until they have a stable one. If we want to move off craco there are a few options to consider (custom/nextjs/remix/vite) so I think this should be a separate task from dependencies upgrade but currently I don't see any reason to do so since this alpha release allow us to upgrade webpack/react-scripts to v5 and thus upgrade react to v18. (the hacky part of this PR is that we are using those but we enforce react 17 but that seem to be OK too). So if we don't want to use this alpha release we need to stick to webpack 4 and react 17 for a while. Thoughts? cc @ramenuncle @cyphertrace @fisprak There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm probably OK with the alpha release if it's temporary and solves a bunch of headaches. |
||
"@elastic/datemath": "^5.0.3", | ||
"@elastic/eui": "^38.2.0", | ||
"@ethers-ancillary/bsc": "^0.0.3", | ||
|
@@ -51,7 +51,9 @@ | |
"@solana/web3.js": "^1.33.0", | ||
"@swim-io/pool-math": "^0.1.1", | ||
"bn.js": "^5.2.0", | ||
"browserify-fs": "^1.0.0", | ||
nicomiicro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"constate": "^3.3.0", | ||
"crypto-browserify": "^3.12.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these browserify modules not provided somehow by CRA? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, see facebook/create-react-app#11756 (comment) |
||
"decimal.js": "^10.3.1", | ||
"dexie": "^3.2.2", | ||
"ethers": "^5.4.4", | ||
|
@@ -61,30 +63,31 @@ | |
"moment": "^2.29.1", | ||
"moralis": "^1.3.5", | ||
"node-sass": "^6", | ||
"path-browserify": "^1.0.1", | ||
"react": "^17.0.2", | ||
"react-dom": "^17.0.2", | ||
"react-query": "3.25.1", | ||
"react-responsive-carousel": "^3.2.23", | ||
"react-router-dom": "^5.2.0", | ||
"react-scripts": "4.0.3", | ||
"wasm-loader": "^1.3.0", | ||
"react-scripts": "^5.0.1", | ||
"stream-browserify": "^3.0.0", | ||
"web-vitals": "^1.0.1", | ||
"zustand": "^4.0.0-rc.1" | ||
}, | ||
"devDependencies": { | ||
"@babel/plugin-proposal-nullish-coalescing-operator": "^7.17.12", | ||
"@sentry/webpack-plugin": "^1.18.8", | ||
"@storybook/addon-actions": "^6.5.2", | ||
"@storybook/addon-docs": "^6.5.2", | ||
"@storybook/addon-essentials": "^6.5.2", | ||
"@storybook/addon-interactions": "^6.5.2", | ||
"@storybook/addon-links": "^6.5.2", | ||
"@storybook/builder-webpack4": "^6.5.2", | ||
"@storybook/manager-webpack4": "^6.5.2", | ||
"@storybook/node-logger": "^6.5.2", | ||
"@storybook/preset-create-react-app": "^3.2.0", | ||
"@storybook/react": "^6.5.2", | ||
"@storybook/testing-library": "^0.0.11", | ||
"@storybook/addon-actions": "^6.5.9", | ||
"@storybook/addon-docs": "^6.5.9", | ||
"@storybook/addon-essentials": "^6.5.9", | ||
"@storybook/addon-interactions": "^6.5.9", | ||
"@storybook/addon-links": "^6.5.9", | ||
"@storybook/builder-webpack5": "^6.5.9", | ||
"@storybook/manager-webpack5": "^6.5.9", | ||
"@storybook/node-logger": "^6.5.9", | ||
"@storybook/preset-create-react-app": "^4.1.2", | ||
"@storybook/react": "^6.5.9", | ||
"@storybook/testing-library": "^0.0.13", | ||
"@testing-library/jest-dom": "^5.11.4", | ||
"@testing-library/react": "^11.1.0", | ||
"@testing-library/react-hooks": "^7.0.2", | ||
|
@@ -110,8 +113,7 @@ | |
"jest-mock-extended": "^2.0.5", | ||
"prettier": "^2.3.2", | ||
"typechain": "^5.1.2", | ||
"typescript": "~4.2", | ||
"webpack": "4.44.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where does our webpack come from now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both |
||
"typescript": "~4.2" | ||
}, | ||
"eslintConfig": { | ||
"overrides": [ | ||
|
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.
Doesn't this imply that webpack should be listed in our dependencies?
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.
Yes and no. Both
react-scripts
andstorybook
depend on webpack. So we are not getting rid of this very easily :DBut adding it to our deps, create a new "maintainance" task of making sure that all these versions match and resolve to one. Why do that when it works like this?
I understand yarn has this issue with binaries that need to be explicitly defined (I assume that's why we have included eslint in our package) but webpack is not used directly thus we don't need to worry about that.
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.
Yeah the yarn issue is what I was thinking about.
I guess because it might stop working at some point and if we've not been explicit it'll be harder to figure out why.