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

Allow faster frontend builds #351

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Jun 30, 2023

New script to build faster, using BUILDSCRIPT=:fast

  • E.g. building locally: BUILDSCRIPT=:fast make build-frontend
  • E.g. building image: BUILDSCRIPT=:fast make images

The drawback is potentially not catching errors related to using dependencies (e.g. patternfly ...); that's the reason why I prefer to keep the default build as a "full compiling" build. But these new targets can save developer's time when doing frequent iterative changes.

In any case, if a compile error is uncaught by the developer, it should be caught by the CI build, which is unchanged.

On my machine, TS compilation is 5x faster (~100s => ~20s)

New script to build faster, using `BUILDSCRIPT=:fast`
- E.g. building locally: `BUILDSCRIPT=:fast make build-frontend`
- E.g. building image: `BUILDSCRIPT=:fast make images`

The drawback is potentially not catching errors related to using
dependencies (e.g. patternfly ...); that's the reason why I prefer to
keep the default build as a "full compiling" build. But these new
targets can save developer's time when doing frequent iterative changes.

In any case, if a compile error is uncaught by the developer, it should
be caught by the CI build, which is unchanged.

On my machine, TS compilation is 5x faster (~100s => ~20s)
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #351 (cf28924) into main (f37382e) will decrease coverage by 0.91%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
- Coverage   58.83%   57.92%   -0.91%     
==========================================
  Files         121      150      +29     
  Lines        4912     6665    +1753     
  Branches      797      797              
==========================================
+ Hits         2890     3861     +971     
- Misses       1894     2588     +694     
- Partials      128      216      +88     
Flag Coverage Δ
uitests 58.83% <ø> (ø)
unittests 55.39% <ø> (?)

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

see 29 files with indirect coverage changes

@jpinsonneau
Copy link
Contributor

From what I see in the ts-node usage the ignore option should default to node_modules that contains @openshift-console folder.

This -I '/node_modules/(?!(@openshift-console)/)/' was introduced since the first PR to include /node_modules/@openshift-console/* folders.
@OlivierCazade do you remember the reason ?

The logging plugin that also use ts-node doesn't add this -I option:
https://github.com/openshift/logging-view-plugin/blob/main/web/package.json#L19

Maybe it's simply not needed anymore ?

@jotak
Copy link
Member Author

jotak commented Jul 3, 2023

@jpinsonneau hmm my first guess was that ignoring node_modules would make it miss compilation issues when calling libraries, but it seems that's not the case, I just did a try adding somethingdoesntexist={true} to a patternfly component, and compiling with make build-frontend BUILDSCRIPT=:fast still catches the error ... so yeah maybe we can completely get rid of this config, and not just as a new build flag.

web/package.json Outdated
@@ -5,12 +5,14 @@
"scripts": {
"clean": "rm -rf ./dist",
"build": "npm run clean && NODE_ENV=production npm run ts-node ./node_modules/.bin/webpack",
"build:fast": "npm run ts-node-fast ./node_modules/.bin/webpack",
Copy link
Member Author

Choose a reason for hiding this comment

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

actually, what makes it faster is perhaps more the removal of npm run clean && NODE_ENV=production than the ts-node options ....

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the NODE_ENV=production part makes the difference:

  • 21677ms without it
  • 51624ms with it

Using npm run clean doesn't change a lot the total time (around 2000ms) so I would suggest to keep it as it ensure the dist folder is recreated from scratch and avoid any cached bundle / manifest / asset

Copy link
Contributor

Choose a reason for hiding this comment

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

going deeper the production is slower mainly because of module.exports.optimization.minimize

- Ignoring node_module with ts-node (default) makes sense for all builds, not just fast build
- Use :dev instead of :fast then, since the only difference with normal
  builds is actually the NODE_ENV=production var
- Remove .git from the dockerfile to avoid rebuilding all after any git
  change (e.g. git fetch)
- As a consequence of the Docker build being unaware of git, need to
  pass some info (version from git) as a docker argument (LDFLAGS)
@jotak
Copy link
Member Author

jotak commented Jul 4, 2023

@jpinsonneau so I did that, removed the -I stuff for ts-node, for all builds. And renamed :fast to :dev as now the difference is really about the NODE_ENV=production variable.
Last thing, I also removed .git from the Dockerfile recipe, as it's causing rebuilding the whole backend for bad reasons (e.g. run a git fetch and you need to rebuild go) - git is only used to infer the version string within the binary. So this is now passed as a docker argument.

Copy link
Contributor

@jpinsonneau jpinsonneau left a comment

Choose a reason for hiding this comment

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

LGTM thanks @jotak 🥳

@jotak
Copy link
Member Author

jotak commented Jul 7, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 7, 2023
@openshift-merge-robot openshift-merge-robot merged commit 13382bf into netobserv:main Jul 7, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants