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(web) restore type checking for production build #1625

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Sep 21, 2024

Problem

Yesterday we found that production build was failing at OBS because type checking. See #1623.

There were some valid complaints because of wrong reference and missing @testing-library/dom dependency overlooked during latest migration to latest dependencies versions #1612 However, the build was failing with a bunch of unexpected errors that were not thrown neither on CI nor in our development environments.

[tsl] ERROR in /agama/web/src/components/users/UsersPage.tsx(35,8)
      TS2741: Property 'children' is missing in type '{}' but required in type '{ [x: string]: any; hasGutter?: boolean; children: any; }'.

Unexpected because claimed children were actually there, but as nested JSX nodes instead of regular prop. Remember,

When you nest content inside a JSX tag, the parent component will receive that content in a prop called children.

https://react.dev/learn/passing-props-to-a-component#passing-jsx-as-children

This made us suspect about @types/react where a type definition hinting TypeScript about such an special prop is expected. However, since we have no clue about those React advanced types and were in a hurry, we took the shortcut to temporary disable the type checking in order to get the production build working again.

Further investigation shows we were on the right track since the issue only occurs when @types/react isn't present. The OBS build is using --legacy-peer-deps, which does an slightly different dependencies installation.

diff --git a/web/package-lock.json b/web/package-lock.json
index aef5ecc90..45f319bc2 100644
--- a/web/package-lock.json
+++ b/web/package-lock.json
@@ -5200,15 +5200,6 @@
         "@types/node": "*"
       }
     },
-    "node_modules/@types/prop-types": {
-      "version": "15.7.12",
-      "resolved": "https://registry.npmjs.org/@types/prop-types/-/prop-types-15.7.12.tgz",
-      "integrity": "sha512-5zvhXYtRNRluoE/jAp4GVsSduVUzNWKkOZrCDBWYtE7biZywwdC2AcEzg+cSMLFRfVgeAFqpfNabiPjxFddV1Q==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true
-    },
     "node_modules/@types/qs": {
       "version": "6.9.16",
       "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.9.16.tgz",
@@ -5223,31 +5214,6 @@
       "dev": true,
       "license": "MIT"
     },
-    "node_modules/@types/react": {
-      "version": "18.3.5",
-      "resolved": "https://registry.npmjs.org/@types/react/-/react-18.3.5.tgz",
-      "integrity": "sha512-WeqMfGJLGuLCqHGYRGHxnKrXcTitc6L/nBUWfWPcTarG3t9PsquqUMuVeXZeca+mglY4Vo5GZjCi0A3Or2lnxA==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true,
-      "dependencies": {
-        "@types/prop-types": "*",
-        "csstype": "^3.0.2"
-      }
-    },
-    "node_modules/@types/react-dom": {
-      "version": "18.3.0",
-      "resolved": "https://registry.npmjs.org/@types/react-dom/-/react-dom-18.3.0.tgz",
-      "integrity": "sha512-EhwApuTmMBmXuFOikhQLIBUn6uFg81SwLMOAUgodJF14SOBOCMdU04gDoYi0WOJJHD144TL32z4yDqCW3dnkQg==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true,
-      "dependencies": {
-        "@types/react": "*"
-      }
-    },
     "node_modules/@types/retry": {
       "version": "0.12.2",
       "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.2.tgz",
@@ -8461,15 +8427,6 @@
         "node": ">=14"
       }
     },
-    "node_modules/csstype": {
-      "version": "3.1.3",
-      "resolved": "https://registry.npmjs.org/csstype/-/csstype-3.1.3.tgz",
-      "integrity": "sha512-M1uQkMl8rQK/szD0LNhtqxIPLpimGm8sOBwU7lLnCpSbTyY3yeU1Vc7l4KT5zT4s/yOxHH5O7tIuuLOCnLADRw==",
-      "dev": true,
-      "license": "MIT",
-      "optional": true,
-      "peer": true
-    },
     "node_modules/data-urls": {
       "version": "4.0.0",
       "resolved": "https://registry.npmjs.org/data-urls/-/data-urls-4.0.0.tgz",

Now, we were able to reproduce the issue faced at OBS.

Solution

The fix is quite simple and clear: to explicitly add the @types/react dev dependency. But this PR adds @types/react-dom too, to avoid similar silly build issues.

Testing

To make sure this is the right fix, below tests has been performed against master branch after restoring the type checking for production build (transpileOnlye: development at webpack config file).

  • NODE_ENV=production npm run build does not fail (because @types/react dep is present)
  • Remove node_modules and run an npm install --legacy-peer-deps: the NODE_ENV=production npm run build fails (because @types/react is not present)
  • Add @types/react dev dep explicitly (npm install -D @types/react)
  • Remove node_modules and run an npm install --legacy-peer-deps again: the NODE_ENV=production npm run build ends successfully.

Open questions

@dgdavid dgdavid marked this pull request as ready for review September 22, 2024 09:26
@dgdavid dgdavid merged commit df39cd5 into master Sep 24, 2024
2 checks passed
@dgdavid dgdavid deleted the fix-dev-deps branch September 24, 2024 09:37
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