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.

Sign up for GitHub

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

Jump to bottom

Fix build #1623

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Fix build #1623

merged 4 commits into from
Sep 20, 2024

Conversation

imobachgs
Copy link
Contributor

agama and agama-web-ui are failing to build. This branch includes a set of fixes.

  • fix(web): fix wrong reference to HTTPClient
  • fix(rust): fix format of profile.schema.json
  • fix(web): add @testing-library/dom
  • fix(web): disable type checking for ts-loader

We need to investigate especially the ts-loader problem, because it did not happen neither on CI
nor in our development machines.

@coveralls
Copy link

Coverage Status

coverage: 71.951%. remained the same
when pulling 5b6c9de on fix-build
into 574a1a5 on master.

dgdavid
dgdavid approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

If manual testing works, you have my LGTM even though it's in draft yet. Ping me if you endup changing it too much.

@imobachgs imobachgs marked this pull request as ready for review September 20, 2024 14:39
@imobachgs imobachgs merged commit 4d0d973 into master Sep 20, 2024
3 checks passed
@imobachgs imobachgs deleted the fix-build branch September 20, 2024 14:39
@dgdavid dgdavid mentioned this pull request Sep 21, 2024
Merged
dgdavid added a commit that referenced this pull request Sep 24, 2024
@dgdavid
fix(web) restore type checking for production build (#1625)
df39cd5 
## 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`](https://www.npmjs.com/package/@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
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](https://github.com/openSUSE/agama/blob/37733ab42b9f438ee7893775fd57b9cf05d392bb/web/webpack.config.js#L176)).

* `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

* Should CI been updated for using the same `npm install` flags than OBS
in an attempt to try catching these issues soon?
* A bit unrelated with this: should we try to go ahead without
`babel-loader`? It still there after start using TypeScript because

> ts-loader works very well in combination with
[babel](https://babeljs.io/) and
[babel-loader](https://github.com/babel/babel-loader).
   > 
   > https://github.com/TypeStrong/ts-loader?tab=readme-ov-file#babel

But maybe we could re-evaluate if we actually need both or could use
`ts-loader` only instead. If so, please be aware of
https://github.com/TypeStrong/ts-loader?tab=readme-ov-file#faster-builds
and https://github.com/TypeStrong/fork-ts-checker-webpack-plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgdavid dgdavid dgdavid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@imobachgs @coveralls @dgdavid