Upgrade dependencies webpack craco #140

nicomiicro · 2022-06-24T20:11:19Z

Description

PR Checklist

The relevant Github Project (and/or label) has been assigned (applies only when there is one)
Preview deployment is not broken. Please visit the Cloudflare pages preview URL

Post Review Checklist

If your PR changes are significant please share the preview deployment URL with the product team in Slack's #product-swim for some manual testing before merging.

Is there a ticket for this change? If yes, please paste the notion link below

Notion link: https://www.notion.so/exsphere/Monthly-dependencies-upgrade-f92aeba4676b4fa9b1a2bb359240a746

cloudflare-workers-and-pages · 2022-06-24T20:16:34Z

Deploying with Cloudflare Pages

Latest commit:	`ce8e746`
Status:	✅ Deploy successful!
Preview URL:	https://18603a8f.swim-ui.pages.dev
Branch Preview URL:	https://upgrade-dependencies-webpack.swim-ui.pages.dev

View logs

github-actions · 2022-06-24T20:23:13Z

✨ Deployment complete! Take a peek over at https://4e85fff9.ui-storybook.pages.dev

due to cloudflare file size limits this shouldn't be a problem if we re-enable chunks

nicomiicro · 2022-06-24T21:02:46Z

@arvakr this also fixes the dependabot alert. The link might not work cause yarn.lock is collapsed.

nicomiicro · 2022-06-24T21:04:00Z

I tested it by doing a swap from Ethereum to Solana in devnet, so I'd assume that if something was wrong on the wasm dependencies I would get an error.

nicomiicro · 2022-06-24T21:05:28Z

Next step would be the react upgrade which will probably require more testing.

wormat

Looks nice but I have a bunch of questions.

wormat · 2022-06-24T21:47:11Z

apps/ui/src/images/social/twitter.svg

What changed these image files?

I did :)
I was getting errors like this one:

ERROR in ./src/images/social/twitter.svg Module build failed (from ./node_modules/@svgr/webpack/lib/index.js): SyntaxError: unknown: Namespace tags are not supported by default. React's JSX doesn't support namespace tags. You can set `throwIfNamespace: false` to bypass this warning. 11 | strokeLinejoin: "round", 12 | strokeMiterlimit: 2 > 13 | }} viewBox="0 0 512 512" width="100%" xmlSpace="preserve" xmlns="http://www.w3.org/2000/svg" xmlns:serif="http://www.serif.com/" xmlnsXlink="http://www.w3.org/1999/xlink" ref={svgRef} aria-labelledby={titleId} {...props}>{title ? <title id={titleId}>{title}</title> : null}<rect height={400} style={{ | ^^^^^^^^^^^ 14 | fill: "none" 15 | }} width={400} x={56} y={56} /><path d="M161.014,464.013c193.208,0 298.885,-160.071 298.885,-298.885c0,-4.546 0,-9.072 -0.307,-13.578c20.558,-14.871 38.305,-33.282 52.408,-54.374c-19.171,8.495 -39.51,14.065 -60.334,16.527c21.924,-13.124 38.343,-33.782 46.182,-58.102c-20.619,12.235 -43.18,20.859 -66.703,25.498c-19.862,-21.121 -47.602,-33.112 -76.593,-33.112c-57.682,0 -105.145,47.464 -105.145,105.144c0,8.002 0.914,15.979 2.722,23.773c-84.418,-4.231 -163.18,-44.161 -216.494,-109.752c-27.724,47.726 -13.379,109.576 32.522,140.226c-16.715,-0.495 -33.071,-5.005 -47.677,-13.148l0,1.331c0.014,49.814 35.447,93.111 84.275,102.974c-15.464,4.217 -31.693,4.833 -47.431,1.802c13.727,42.685 53.311,72.108 98.14,72.95c-37.19,29.227 -83.157,45.103 -130.458,45.056c-8.358,-0.016 -16.708,-0.522 -25.006,-1.516c48.034,30.825 103.94,47.18 161.014,47.104" style={{ 16 | fill: "#1da1f2", at File.buildCodeFrameError (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/core/lib/transformation/file/file.js:249:12) at NodePath.buildCodeFrameError (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/path/index.js:143:21) at PluginPass.JSXNamespacedName (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/plugin-transform-react-jsx/lib/create-plugin.js:86:24) at newFn (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/visitors.js:177:21) at NodePath._call (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/path/context.js:53:20) at NodePath.call (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/path/context.js:40:17) at NodePath.visit (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/path/context.js:100:31) at TraversalContext.visitQueue (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/context.js:103:16) at TraversalContext.visitSingle (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/context.js:77:19) at TraversalContext.visit (/Users/nico/Code/swim/swim/apps/ui/node_modules/@babel/traverse/lib/context.js:131:19) webpack compiled with 1 error and 3 warnings

I can look into that throwIfNamespace flag but it was easier for me to just remove a few namespaces from the SVGs. Thoughts?

I have absolutely no knowledge of best practice here. Happy with the change as long as nobody has any objections.

wormat · 2022-06-24T21:49:34Z

apps/ui/package.json

-    "build": "craco build",
-    "release": "SENTRY_RELEASE=$(git rev-parse --short HEAD) SKIP_PREFLIGHT_CHECK=true DISABLE_ESLINT_PLUGIN=true craco build",
+    "build": "SKIP_PREFLIGHT_CHECK=true DISABLE_ESLINT_PLUGIN=true craco build",
+    "release": "SENTRY_RELEASE=$(git rev-parse --short HEAD) SKIP_PREFLIGHT_CHECK=true 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'",


Hmm, why wasn't this necessary before? Is it related to the Sentry issues with broken sourcemaps?

hmm, good catch. I can't remember why I added it, could be because I saw it in release. Turns out we don't need it anymore and it's probably required due to our inclusion of webpack in our package.json? Removed in d7f6b65

I was talking about SKIP_PREFLIGHT_CHECK but it turns out we still need DISABLE_ESLINT_PLUGIN and that was missing from the build script. It fails on master too, probably nobody uses that.

22:53:47.075 | [eslint] Plugin "react" was conflicted between ".eslintrc.json" and "BaseConfig » /opt/buildhome/repo/apps/ui/node_modules/react-scripts/node_modules/eslint-config-react-app/base.js".

hmm, just realised that you are probably asking about the sourcemaps deletion. Our main sourcemap file was just below the 25MB cloudflare file limit with webpack 4 so the deployment would succeed with no issues. But with webpack 5 that jumped to 35MB. Not sure why and I couldn't find any reference, our bundle size increased by ~0.1MB too. But to answer your question the rm is done so that we don't hit that 25MB file limit of cloudflare.

This change is an attempt to fix that sourcemap issue (where the source is missing) and unfortunately (sorry) it's hidden and lacks its own commit 😞

Haha yes I was talking about the sourcemaps, sorry should have been clearer. So will we no longer have source map support at all then? I know we can send the source to Sentry separately but it's nice for debugging in the browser sometimes.

We do send them in Sentry so error tracking is not affected. But yes it gets a bit inconvenient since you will have to manually add them if you need to debug in production. Also note that this issue will probably go away once (and if) we re-enable route base chunks.

There was also another suggestion from @fisprak to try devtool: 'nosources-source-map' but I haven't tried that.

Just mentioning that source maps in production are incorrect now, unfortunately. When I set a breakpoint on swim.io and perform actions on the website, devtools point to an incorrect code.

wormat · 2022-06-24T21:58:08Z

apps/ui/package.json

@@ -35,7 +35,7 @@
  },
  "dependencies": {
    "@certusone/wormhole-sdk": "^0.2.6",
-    "@craco/craco": "^6.3.0",
+    "@craco/craco": "7.0.0-alpha.3",


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?

We definitely thought about that since we believed they won't be releasing a new version with create-react-app/react-scripts 5 support but it looks like they are working on it and they are already have a package that works for us.

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

I'm probably OK with the alpha release if it's temporary and solves a bunch of headaches.

apps/ui/package.json

wormat · 2022-06-24T22:00:09Z

apps/ui/package.json

    "constate": "^3.3.0",
+    "crypto-browserify": "^3.12.0",


Are these browserify modules not provided somehow by CRA?

Nope, see facebook/create-react-app#11756 (comment)
many complaints on that issue...

wormat · 2022-06-24T22:00:30Z

apps/ui/package.json

@@ -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"


Where does our webpack come from now?

Both react-scripts and storybook have it as a dependency.

wormat · 2022-06-24T22:01:01Z

apps/ui/.storybook/main.js

@@ -1,3 +1,5 @@
+const webpack = require("webpack");


Doesn't this imply that webpack should be listed in our dependencies?

Yes and no. Both react-scripts and storybook depend on webpack. So we are not getting rid of this very easily :D
But 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.

Yeah the yarn issue is what I was thinking about.

Why do that when it works like this?

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.

wormat · 2022-06-24T22:06:07Z

apps/ui/craco.config.js

-        test: /\.wasm$/,
-        include: /node_modules\/(bridge|token-bridge)/,
-        loaders: ["wasm-loader"],
+      webpackConfig.experiments = {


Ooh, how much confidence should we have in this?

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.

apps/ui/craco.config.js

wormat · 2022-06-24T22:06:51Z

apps/ui/craco.config.js

+            fs: require.resolve("browserify-fs"),
+            path: require.resolve("path-browserify"),
+            stream: require.resolve("stream-browserify"),
+            crypto: require.resolve("crypto-browserify"),


Ah I see, they were included by default in webpack v4 but removed in v5?

Exactly, all of them were added based on webpack's own output. They guide on adding the polyfills your bundle needs.

Co-authored-by: Will <[email protected]>

nicomiicro · 2022-06-27T19:02:06Z

Discussion in slack on why we are not going forward with this now.

nicomiicro added 2 commits June 24, 2022 18:07

chore(ui): remove webpack from dependencies

a4392f8

chore(ui): upgrade webpack/react-scripts/craco

9a789ea

nicomiicro added the dependencies Pull requests that update a dependency file label Jun 24, 2022

nicomiicro self-assigned this Jun 24, 2022

build(ui): delete sourcemaps after release

5c36620

due to cloudflare file size limits this shouldn't be a problem if we re-enable chunks

nicomiicro marked this pull request as ready for review June 24, 2022 21:02

nicomiicro requested review from wormat, ramenuncle, arvakr, cyphertrace and fisprak June 24, 2022 21:02

wormat reviewed Jun 24, 2022

View reviewed changes

nicomiicro and others added 3 commits June 26, 2022 22:24

chore(ui): typo

5405c08

Co-authored-by: Will <[email protected]>

refactor(ui): remove SKIP_PREFLIGHT_CHECK flag

d7f6b65

build(ui): add DISABLE_ESLINT_PLUGIN=true back

ce8e746

nicomiicro closed this Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade dependencies webpack craco #140

Upgrade dependencies webpack craco #140

nicomiicro commented Jun 24, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 24, 2022 •

edited

Loading

nicomiicro commented Jun 24, 2022 •

edited

Loading

nicomiicro commented Jun 24, 2022

nicomiicro commented Jun 24, 2022

wormat left a comment

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

wormat Jun 27, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

nicomiicro Jun 26, 2022

nicomiicro Jun 26, 2022

wormat Jun 27, 2022

nicomiicro Jun 27, 2022

nicomiicro Jun 27, 2022

fisprak Jun 27, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

wormat Jun 27, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022 •

edited

Loading

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

wormat Jun 27, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

wormat Jun 24, 2022

nicomiicro Jun 26, 2022

nicomiicro commented Jun 27, 2022

Upgrade dependencies webpack craco #140

Upgrade dependencies webpack craco #140

Conversation

nicomiicro commented Jun 24, 2022 • edited Loading

Description

PR Checklist

Post Review Checklist

Is there a ticket for this change? If yes, please paste the notion link below

cloudflare-workers-and-pages bot commented Jun 24, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Jun 24, 2022 • edited Loading

nicomiicro commented Jun 24, 2022 • edited Loading

nicomiicro commented Jun 24, 2022

nicomiicro commented Jun 24, 2022

wormat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicomiicro Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicomiicro commented Jun 27, 2022

nicomiicro commented Jun 24, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 24, 2022 •

edited

Loading

nicomiicro commented Jun 24, 2022 •

edited

Loading

nicomiicro Jun 26, 2022 •

edited

Loading