Updating build toolchain and deps of Web App reporter #7952

tsteenbe · 2023-11-28T20:13:40Z

This pull request contains a significant changes to how ORT's WebApp (template) is generated and upgrade all dependencies to the latest available versions.

After @mennaElnemr9 created #6598 and #6552 I reviewed the impact of updating dependencies which let me to find issue after issues. Several build toolchain and app dependencies have introduced breaking changes or are not longer maintained. After a lots of research I came to the conclusion major rewrite of the WebApp was needed. Especially use create-react-app (CRA) and rescripts was problematic since rescripts has been archived and updated CRA ORT WebApp builds showed me several deprecated dependencies errors and warnings.

My current plan (subject to change):
Step 1: Change build toolchain and update deps
Step 2: Refactor React component code to use React v18 ways of working e.g. replace component classes with functional components. Possible look into replacing dependency on react-syntax-highlighter
Step 3: Implement new features / bug fixes

codecov · 2023-11-28T20:26:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.93%. Comparing base (9707529) to head (e4e2a72).
Report is 15 commits behind head on main.

❗ Current head e4e2a72 differs from pull request most recent head b823a30. Consider uploading reports for the commit b823a30 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7952      +/-   ##
============================================
- Coverage     67.69%   66.93%   -0.77%     
- Complexity     1000     2049    +1049     
============================================
  Files           247      357     +110     
  Lines          7904    17081    +9177     
  Branches        876     2449    +1573     
============================================
+ Hits           5351    11434    +6083     
- Misses         2173     4625    +2452     
- Partials        380     1022     +642

Flag	Coverage Δ
funTest-docker	`66.05% <ø> (+0.61%)`	⬆️
funTest-non-docker	`34.35% <ø> (-0.50%)`	⬇️
test	`36.30% <ø> (-1.18%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

heliocastro

Looks good to me
Maybe on a second pass you can extend eslint verifications, but for this upgrade no need now.

This is a draft still.

tsteenbe · 2023-12-04T10:32:13Z

Looks good to me Maybe on a second pass you can extend eslint verifications, but for this upgrade no need now.

@heliocastro Added an eslint config, was already working on it when you wrote your comment but took me a couple of days to get the various rules set up.

plugins/reporters/web-app-template/build.gradle.kts

.ort.yml

plugins/reporters/web-app-template/.editorconfig

plugins/reporters/web-app-template/package.json

sschuberth · 2024-03-07T08:43:14Z

@tsteenbe can we please move forward with this? Or should some one take over?

heliocastro · 2024-03-07T13:09:11Z

@tsteenbe if you still in the middle of the moving process for some time, i can take over these one.

heliocastro

Good to Go, even with recent changes

fviernau · 2024-04-16T07:39:41Z

I've tested it as well by using two evaluation-result.json files and comparing old vs. new report.
I've used Firefox version 123 under Ubuntu.
I've found a couple of nice formatting improvements.

Besides these, I was not sure about these formatting changes :

Margin between tab icon and text:

old:
new:

Same here:

old:
new:

Key of the key value pairs now gray:

old:
new:

Also I've found a bug with the filtering of findings in scan results. This can be reproduced in
multiple places where the scan result details are shown. E.g. in the tree tab, in the issues tab and so forth:

old:
new:

My suggestion would be to at minimum fix the bug, plus the changed margins. I'm not sure about the graying out of the key.

Update versions of Node and Yarn used in WebApp's build scripts to the versions used in the official ORT Docker image[1]. [1]: https://github.com/oss-review-toolkit/ort/blob/9707529/.versions Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Replace the create-react-app[1] and the rescripts[2] based build toolchain with vite[3] and vite-plugin-singlefile[4] as: - rescripts project has been archived. - continue using create-react-app is not viable due to upcoming API and packages deprecations. - vite has good community support, it builds faster and its toolchain comes out-of-the-box with almost everything we need. [1]: https://github.com/facebook/create-react-app [2]: https://github.com/harrysolovay/rescripts [3]: https://vitejs.dev/ [4]: https://github.com/richardtallent/vite-plugin-singlefile Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Fix up formatting and indentation of JSON ORT result data to 4 spaces so it's aligned with other WebApp code. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Update all dependencies to latest available versions except Ant Design (antd) whose latest version has breaking API changes. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

sschuberth · 2024-04-16T07:48:02Z

I've tested it as well by using two evaluation-result.json files and comparing old vs. new report.

Thanks for the effort you've put into testing this!

tsteenbe · 2024-04-16T10:16:34Z

@fviernau All your comments have been address. The filtering issue comes from a bug in upstream Ant Design (wrong input data is returned on setting of filters). I have now downgraded the version of Ant Design used to a version without the bug - will fill a bug report upstream.

Upgrade web app code to use Ant Design (antd) v5 and update ORT WebApp code to migrate off deprecated antd APIs. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Improve code linting by defining plugins and rules in an eslint configuration file. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Automatically resolve issues flagged by eslint by running `eslint . --fix`. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Resolve warning from Vite compiler that `//` should be replaced by `/* */`. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Fix up check used to determine WebApp report is template used string comparison which in some cases incorrectly evaluated to true. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

Per MDN docs for the viewport meta tag [1] the default for initial-scale is 1 so no needed to include it. [1]: https://developer.mozilla.org/en-US/docs/Web/HTML/Viewport_meta_tag Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

sschuberth · 2024-04-16T10:25:17Z

I debugged the filtering issue and it bug in upstream Ant Design

Could you please link the upstream issue (or create one if none exists yet) so we can upvote / subscribe to it?

tsteenbe · 2024-04-16T10:45:01Z

Could you please link the upstream issue (or create one if none exists yet) so we can upvote / subscribe to it?

Will do. Did already a quick check of the Ant Design issues (which is easy as many reports are in Mandarin) issue doesn't seem to be reported. Which does not surprise me as I am struggling to reproduce the bug outside of ORT - bug with a small reproducable example usually get fixed fast by Ant Design maintainers.

fviernau · 2024-04-16T11:43:57Z

I've tested again (similar as above) with latest updated (ba81082).
My previous findings are now all fixed.

Approving as it seems to work nicely, computes a bit faster and reports are smaller (without understanding every single JS detail).

heliocastro

Results are same as previous iteraction, so good to go.

As of [1] the web-app reporter build became much faster. The overhead to set up a new job now is bigger than the actual build time in most of the cases / for incremental builds, so include the web-app reporter build into the regular build again to simplify the setup. [1]: #7952 Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

tsteenbe force-pushed the webapp-reporter-v0.3 branch from 7f1585d to 0635e08 Compare November 28, 2023 20:31

This was referenced Nov 28, 2023

deps(reporter-web-app): Update React packages #6598

Closed

deps(reporter-web-app): Update antd package #6552

Closed

tsteenbe force-pushed the webapp-reporter-v0.3 branch 4 times, most recently from 4f5759e to c316a5f Compare November 30, 2023 06:49

heliocastro previously approved these changes Nov 30, 2023

View reviewed changes

tsteenbe force-pushed the webapp-reporter-v0.3 branch from c316a5f to 2b1998d Compare November 30, 2023 09:29

heliocastro self-requested a review November 30, 2023 09:57

tsteenbe force-pushed the webapp-reporter-v0.3 branch 6 times, most recently from a2c9736 to c27b11e Compare December 4, 2023 10:19

tsteenbe marked this pull request as ready for review December 4, 2023 10:22

tsteenbe requested a review from a team as a code owner December 4, 2023 10:22

tsteenbe force-pushed the webapp-reporter-v0.3 branch 3 times, most recently from 65cb16f to e4e2a72 Compare December 4, 2023 10:30

sschuberth reviewed Dec 4, 2023

View reviewed changes

plugins/reporters/web-app-template/build.gradle.kts Show resolved Hide resolved

.ort.yml Show resolved Hide resolved

plugins/reporters/web-app-template/.editorconfig Show resolved Hide resolved

plugins/reporters/web-app-template/package.json Outdated Show resolved Hide resolved

sschuberth requested a review from a team February 12, 2024 09:15

tsteenbe force-pushed the webapp-reporter-v0.3 branch 2 times, most recently from b823a30 to a4b8872 Compare April 9, 2024 19:57

heliocastro previously approved these changes Apr 15, 2024

View reviewed changes

tsteenbe added 5 commits April 16, 2024 09:46

style(web-app-template): Add an .editorconfig file

f88c039

Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

style(web-app-template): Reformat JSON

47df10e

Fix up formatting and indentation of JSON ORT result data to 4 spaces so it's aligned with other WebApp code. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

deps(web-app-template): Various version updates

4e79030

Update all dependencies to latest available versions except Ant Design (antd) whose latest version has breaking API changes. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

tsteenbe dismissed heliocastro’s stale review via 301d1e5 April 16, 2024 07:53

tsteenbe force-pushed the webapp-reporter-v0.3 branch 2 times, most recently from 301d1e5 to b00be3c Compare April 16, 2024 10:11

tsteenbe added 6 commits April 16, 2024 12:20

deps(web-app-template): Migrate to Ant Design v5

aa9e58c

Upgrade web app code to use Ant Design (antd) v5 and update ORT WebApp code to migrate off deprecated antd APIs. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

test(web-app-template): Add eslint config

2da6978

Improve code linting by defining plugins and rules in an eslint configuration file. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

fix(web-app-template): Auto-resolve eslint issues

a148ab7

Automatically resolve issues flagged by eslint by running `eslint . --fix`. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

fix(web-app-template): Resolve build warning

7d192a2

Resolve warning from Vite compiler that `//` should be replaced by `/* */`. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

fix(web-app-template): Fix DOM invalid string error

0d9a3e1

Fix up check used to determine WebApp report is template used string comparison which in some cases incorrectly evaluated to true. Signed-off-by: Thomas Steenbergen <opensource@steenbe.nl>

tsteenbe force-pushed the webapp-reporter-v0.3 branch from b00be3c to ba81082 Compare April 16, 2024 10:21

tsteenbe requested a review from heliocastro April 16, 2024 10:26

fviernau approved these changes Apr 16, 2024

View reviewed changes

heliocastro approved these changes Apr 16, 2024

View reviewed changes

tsteenbe merged commit ed7fdbe into main Apr 16, 2024
21 checks passed

tsteenbe deleted the webapp-reporter-v0.3 branch April 16, 2024 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating build toolchain and deps of Web App reporter #7952

Updating build toolchain and deps of Web App reporter #7952

tsteenbe commented Nov 28, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

heliocastro left a comment

tsteenbe commented Dec 4, 2023

sschuberth commented Mar 7, 2024

heliocastro commented Mar 7, 2024

heliocastro left a comment

fviernau commented Apr 16, 2024 •

edited

Loading

sschuberth commented Apr 16, 2024

tsteenbe commented Apr 16, 2024 •

edited

Loading

sschuberth commented Apr 16, 2024

tsteenbe commented Apr 16, 2024

fviernau commented Apr 16, 2024

heliocastro left a comment

Updating build toolchain and deps of Web App reporter #7952

Updating build toolchain and deps of Web App reporter #7952

Conversation

tsteenbe commented Nov 28, 2023

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

heliocastro left a comment

Choose a reason for hiding this comment

tsteenbe commented Dec 4, 2023

sschuberth commented Mar 7, 2024

heliocastro commented Mar 7, 2024

heliocastro left a comment

Choose a reason for hiding this comment

fviernau commented Apr 16, 2024 • edited Loading

sschuberth commented Apr 16, 2024

tsteenbe commented Apr 16, 2024 • edited Loading

sschuberth commented Apr 16, 2024

tsteenbe commented Apr 16, 2024

fviernau commented Apr 16, 2024

heliocastro left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2023 •

edited

Loading

fviernau commented Apr 16, 2024 •

edited

Loading

tsteenbe commented Apr 16, 2024 •

edited

Loading