Improve frontend-building related Makefile rules #32753

StCyr · 2022-06-08T09:48:52Z

Currently it's necessary to run npm run sass:icons in addition to make dev-setup and make build-js, to have the icons on your development server.

This PR improves the frontend-building related Makefile rules to make this additional manual action unneeded anymore

Signed-off-by: Cyrille Bollu [email protected]

CarlSchwan · 2022-06-08T09:51:21Z

Makefile

+watch-js: build-css
 	npm run watch


Suggested change

watch-js: build-css

npm run watch

watch-js: build-css

npm run watch &

npm run sass:watch

Should make it possible to watch both at the same time

Mind though that I don't run the npm run sass commmand because it gives me errors here (see my message on Talk)

Ok, I've made the requested change

StCyr · 2022-06-14T10:25:17Z

I don't think the drone failure is due to my changes

skjnldsv · 2022-06-14T15:13:44Z

I think we should add the sass commands as a postbuild step instead ?
Makefiles are arbitrary and not ideal imho

StCyr · 2022-06-20T19:15:47Z

I think we should add the sass commands as a postbuild step instead ? Makefiles are arbitrary and not ideal imho

I don't understand your comment :-(

skjnldsv · 2022-06-21T10:48:39Z

I don't understand your comment :-(

https://docs.npmjs.com/cli/v8/using-npm/scripts#pre--post-scripts

  "scripts": {
    "build": "NODE_ENV=production webpack --progress --config webpack.prod.js",
    "postbuild": "npm run sass && npm run sass:icons",
    "sass": "sass --load-path core/css core/css/ apps/*/css",
    "sass:icons": "babel-node core/src/icons.js"
  }

PVince81 · 2022-07-22T17:12:38Z

I've lost quite some time believing this was already merged and kept compiling and trying different node versions.
I should have looked at the exact failure to see that it was always about icons.

Anyway, we should push this forward fast because others might bump into the same issues.

the dist/icons.css file. Updates the core/src/icons.js script to allow it to run before 'npm run dev', 'npm run build', or 'npm run watch' is run. Signed-off-by: Cyrille Bollu <[email protected]>

Signed-off-by: Vincent Petry <[email protected]>

PVince81 · 2022-07-22T17:15:23Z

I've rebased this and added the post-build step

next up:

local test

PVince81 · 2022-07-22T17:24:34Z

tested locally, while it does run those commands, there's another separate issue: in README we tell people to run "make build-js-production" and now that would include those commands.

however the scss commands are also exploring the non-core apps and displaying errors for me locally, so nothing gets built.
and this happens right at the end of the time-consuming build. should we make it a pre-build step instead ?

I think we should also limit the scss commands to the apps that are part of the core repo.
Otherwise we need to state in the README that for buildling production assets one needs to delete all non-core apps.

Thoughts ?

PVince81 · 2022-07-22T17:27:52Z

for reference, this is the local output on my setup:

%  npm run sass && npm run sass:icons

> [email protected] sass
> sass --load-path core/css core/css/ apps/*/css

Error: Undefined mixin.
   ╷
26 │ @include icon-black-white('briefcase', 'calendar', 5);
   │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  apps/calendar/css/icons.scss 26:1  root stylesheet

Error: Undefined variable.
   ╷
38 │         margin-bottom: $grid-height-unit;
   │                        ^^^^^^^^^^^^^^^^^
   ╵
  apps/contacts/css/Properties/Properties.scss 38:18  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/deck.scss 1:1  root stylesheet

Error: Undefined mixin.
  ╷
4 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/icons.scss 4:1          @import
  apps/deck/css/globalstyles.scss 27:9  root stylesheet

Error: Undefined mixin.
  ╷
4 │ @include icon-black-white('deck', 'deck', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/deck/css/icons.scss 4:1  root stylesheet

Error: Undefined mixin.
    ╷
308 │ @include icon-black-white('inbox', 'mail', 1);
    │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ╵
  apps/mail/css/mail.scss 308:1  root stylesheet

Error: Undefined variable.
  ╷
1 │ @media only screen and (max-width: $breakpoint-mobile) {
  │                                    ^^^^^^^^^^^^^^^^^^
  ╵
  apps/mail/css/mobile.scss 1:36  root stylesheet
Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.

Recommendation: math.div($size, 210) or calc($size / 210)

More info and automated migrator: https://sass-lang.com/d/slash-div

   ╷
68 │             $sizeY: $size / 210 * 297;
   │                     ^^^^^^^^^^^
   ╵
    apps/officeonline/css/admin.scss 68:12  root stylesheet


Error: Undefined mixin.
   ╷
24 │     @include icon-color('folder', 'filetypes', $color-black, 1, true);
   │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  apps/photos/css/icons.scss 24:2  root stylesheet

Error: Undefined variable.
   ╷
62 │ @media only screen and (max-width: $breakpoint-mobile) {
   │                                    ^^^^^^^^^^^^^^^^^^
   ╵
  apps/serverinfo/css/style.scss 62:36  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1         @import
  apps/spreed/css/merged-files.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1                @import
  apps/spreed/css/merged-public-share.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1              @import
  apps/spreed/css/merged-share-auth.scss 2:9  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('reply', 'spreed', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/spreed/css/icons.scss 1:1   @import
  apps/spreed/css/merged.scss 2:9  root stylesheet

Error: Undefined variable.
   ╷
30 │         border-color: $color-error;
   │                       ^^^^^^^^^^^^
   ╵
  apps/spreed/css/settings-admin.scss 30:17  root stylesheet

Error: Undefined variable.
    ╷
131 │     border-color: $color-error;
    │                   ^^^^^^^^^^^^
    ╵
  apps/support/css/support.scss 131:16  root stylesheet

Error: Undefined mixin.
  ╷
1 │ @include icon-black-white('tasks', 'tasks', 1);
  │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  apps/tasks/css/tasks-talk.scss 1:1  root stylesheet

StCyr · 2022-08-04T11:56:04Z

tested locally, while it does run those commands, there's another separate issue: in README we tell people to run "make build-js-production" and now that would include those commands.

however the scss commands are also exploring the non-core apps and displaying errors for me locally, so nothing gets built. and this happens right at the end of the time-consuming build. should we make it a pre-build step instead ?

I think we should also limit the scss commands to the apps that are part of the core repo. Otherwise we need to state in the README that for buildling production assets one needs to delete all non-core apps.

Thoughts ?

Ah, maybe you've find out why this command has never been included in the building scripts in the first place; It looks quite difficult to solve efficiently (ie: maintaining a list of core apps).

If we are to update the README file, I would prefer to remove the command from the build script altogether and update the README file with something along the line:

--- a/README.md
+++ b/README.md
@@ -74,6 +74,9 @@ make watch-js
 
 # build for production with minification
 make build-js-production
+
+# build icons (caution: will also try to build icons for non-core apps)
+make icons

And create the corresponding Makefile rule.

skjnldsv · 2022-08-10T10:46:21Z

for reference, this is the local output on my setup:

Yes, because you are cloning foreign apps into your server/apps folder
Use apps2 or use a clean server repo :)

PVince81 · 2022-08-10T13:48:57Z

for reference, this is the local output on my setup:

Yes, because you are cloning foreign apps into your server/apps folder Use apps2 or use a clean server repo :)

yes, I've switched to "apps-extra" since, maybe need to add a note about this in the README so other people don't get surprised

PVince81 · 2022-12-06T13:44:23Z

closing in favor of #35622 which uses pure npm approach of post-build

StCyr requested review from skjnldsv and CarlSchwan June 8, 2022 09:48

StCyr self-assigned this Jun 8, 2022

StCyr force-pushed the enhancement/better-build-of-frontend-code branch from 5298946 to a930fef Compare June 8, 2022 09:49

CarlSchwan approved these changes Jun 8, 2022

View reviewed changes

CarlSchwan reviewed Jun 8, 2022

View reviewed changes

StCyr force-pushed the enhancement/better-build-of-frontend-code branch 3 times, most recently from f9c874e to a499a41 Compare June 13, 2022 09:56

skjnldsv added the 2. developing Work in progress label Jun 21, 2022

PVince81 mentioned this pull request Jul 22, 2022

Fix sharing panel lighthouse warnings #33321

Merged

StCyr and others added 2 commits July 22, 2022 19:13

Updates frontend-building related Makefile rules to also create

135c698

the dist/icons.css file. Updates the core/src/icons.js script to allow it to run before 'npm run dev', 'npm run build', or 'npm run watch' is run. Signed-off-by: Cyrille Bollu <[email protected]>

Add postbuild step

f554c33

Signed-off-by: Vincent Petry <[email protected]>

PVince81 force-pushed the enhancement/better-build-of-frontend-code branch from a499a41 to f554c33 Compare July 22, 2022 17:15

PVince81 mentioned this pull request Dec 6, 2022

Automatically build sass and sass:icons after build #35622

Merged

PVince81 closed this Dec 6, 2022

skjnldsv deleted the enhancement/better-build-of-frontend-code branch December 6, 2022 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve frontend-building related Makefile rules #32753

Improve frontend-building related Makefile rules #32753

StCyr commented Jun 8, 2022

CarlSchwan Jun 8, 2022

StCyr Jun 8, 2022

StCyr Jun 10, 2022

StCyr commented Jun 14, 2022

skjnldsv commented Jun 14, 2022 •

edited

Loading

StCyr commented Jun 20, 2022

skjnldsv commented Jun 21, 2022

PVince81 commented Jul 22, 2022

PVince81 commented Jul 22, 2022 •

edited

Loading

PVince81 commented Jul 22, 2022

PVince81 commented Jul 22, 2022

StCyr commented Aug 4, 2022

skjnldsv commented Aug 10, 2022 •

edited

Loading

PVince81 commented Aug 10, 2022

PVince81 commented Dec 6, 2022

Improve frontend-building related Makefile rules #32753

Improve frontend-building related Makefile rules #32753

Conversation

StCyr commented Jun 8, 2022

CarlSchwan Jun 8, 2022

Choose a reason for hiding this comment

StCyr Jun 8, 2022

Choose a reason for hiding this comment

StCyr Jun 10, 2022

Choose a reason for hiding this comment

StCyr commented Jun 14, 2022

skjnldsv commented Jun 14, 2022 • edited Loading

StCyr commented Jun 20, 2022

skjnldsv commented Jun 21, 2022

PVince81 commented Jul 22, 2022

PVince81 commented Jul 22, 2022 • edited Loading

PVince81 commented Jul 22, 2022

PVince81 commented Jul 22, 2022

StCyr commented Aug 4, 2022

skjnldsv commented Aug 10, 2022 • edited Loading

PVince81 commented Aug 10, 2022

PVince81 commented Dec 6, 2022

skjnldsv commented Jun 14, 2022 •

edited

Loading

PVince81 commented Jul 22, 2022 •

edited

Loading

skjnldsv commented Aug 10, 2022 •

edited

Loading