feat(stark-build): implement integration with Angular CLI via @angular-builders #1148

christophercr · 2019-02-22T12:19:58Z

ISSUES CLOSED: #883, #146, #114

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[X] Build related changes
[X] CI related changes
[X] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Stark based apps are not compatible with Angular CLI due to the customizations done in Stark-Build.

Issue Number: #883, #146, #114 #1036

What is the new behavior?

Stark-Build now overrides and expands the default build configuration from Angular CLI to leverage the functionalities needed for the different Stark packages.

Does this PR introduce a breaking change?

[X] Yes
[ ] No

Other information

coveralls · 2019-02-22T15:29:23Z

Coverage decreased (-0.005%) to 93.473% when pulling 9a26c00 on christophercr:feature/angular-cli-custom-builders into 92496bc on NationalBankBelgium:master.

SuperITMan · 2019-02-25T07:45:05Z

showcase/angular.json

@@ -88,7 +115,7 @@
          }
        },
        "serve": {
-          "builder": "@angular-devkit/build-angular:dev-server",
+          "builder": "@angular-builders/dev-server:generic",
          "options": {
            "browserTarget": "showcase:build"


With the previous implementation, the default port was 3000 and by default, the url was opened automatically in the default browser.

What do you think of keeping those 2 configurations ? 😊

We can add them back by adding those 2 options:

"options": { "browserTarget": "showcase:build", "port": 3000, "open": true },

Mmm I was hesitating about that... and now that I think of it, I don't see why should use a different port from the default one that NG CLI uses, don't you think?

On the other hand, I do find it useful to open automatically the browser, so I'll that option

SuperITMan · 2019-02-25T07:46:01Z

starter/angular.json

@@ -84,7 +111,7 @@
          }
        },
        "serve": {
-          "builder": "@angular-devkit/build-angular:dev-server",
+          "builder": "@angular-builders/dev-server:generic",
          "options": {
            "browserTarget": "starter:build"


Same as showcase/angular.json

SuperITMan · 2019-02-25T07:57:37Z

showcase/config/webpack-custom-config.dev.json

@@ -1,4 +1,4 @@
 {
-  "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io",
+  "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io ws://localhost:4200",


This is already managed in wepack-dev-server in webpack-partial.dev.js.
I think we can remove this change 😊

In fact, that was my bad... I think we should not modify the HOST and PORT via environment variables anymore because NG CLI already allows this via an option.

So I guess we need to change the CSP directive in webpack-partial.dev.js like this:
"connect-src 'self' ws://localhost:4200 " + webpackCustomConfig["cspConnectSrc"],

If we can get the default port and default host or the values defined in angular.json (if defined), I think it would be better to keep the previous scenario in cspDirectives in webpack-partial.dev.js --> "connect-src 'self' ws://" + METADATA.HOST + ":" + METADATA.PORT + " " " + webpackCustomConfig["cspConnectSrc"]

True, I'm fixing this

You forgot to remove your change here.
It should be "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io",

SuperITMan · 2019-02-25T08:02:15Z

docs/stark-build/NG_CLI_BUILD_CUSTOMIZATIONS.md

+            "serve": {
+                "builder": "@angular-builders/dev-server:generic",
+                "options": {
+                    "browserTarget": "your-app:build"


If you apply the asked modification about the port and the automatic opening, don't forget to apply it here 😊

I don't think it is necessary to add it in this doc, the host and port are just app specific so in Stark we don't really care about it

SuperITMan · 2019-02-25T08:03:19Z

docs/stark-build/NG_CLI_BUILD_CUSTOMIZATIONS.md

+                },
+                "configurations": {
+                    "hmr": {
+                        "browserTarget": "yourProjectName:build:hmr"


This should be "browserTarget": "your-app:build:hmr"

SuperITMan · 2019-02-25T12:37:34Z

starter/package.json

@@ -26,13 +26,15 @@
    "npm": ">=5.6.0"
  },
  "scripts": {
-    "build:aot:prod": "npm run clean:dist && npm run clean:aot && cross-env BUILD_AOT=1 npm run webpack -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.prod.js  --progress --profile --bail",
+    "build:aot:prod": "npm run clean:dist && npm run clean:aot && npm run ng -- build --aot=true --prod=true --buildOptimizer=true",


Same here about the arguments --aot and --buildOptimizer

SuperITMan · 2019-02-25T12:38:28Z

showcase/src/index.html

-		<% for (let i = 0; i < htmlWebpackPlugin.options.dllFiles.css.length; ++i) { %>
-		<link href="<%= htmlWebpackPlugin.options.dllFiles.css[i] %>" rel="stylesheet" />
-		<% } %> <% } %>
+		<!--<% if (htmlWebpackPlugin.options.dllFiles) { %>-->


Do we plan to have support for dll files later ?

@christophercr What about this ? 😊

SuperITMan · 2019-02-25T12:48:42Z

packages/stark-build/config/build-utils.js

+	IS_DEV_SERVER: helpers.hasProcessFlag("serve"),
+	HMR: helpers.hasProcessFlag("hmr"),
+	AOT: helpers.hasProcessFlag("aot"),
+	E2E: helpers.hasProcessFlag("e2e"),


I don't see anywhere in our current package.json the flag --e2e. Can you confirm it is correct ? 😊

Indeed, the flag is not used... I kept it just in case... we should find a way to know whether the build is for e2e... but I think we can do that later

SuperITMan · 2019-02-25T12:50:37Z

packages/stark-build/config/webpack-partial.dev.js

+	PORT: process.env.PORT || 4200,
+	ENV: isProd ? "production" : "development",
+	environment: helpers.hasProcessFlag("hmr") ? "hmr" : "dev",
+	WATCH: !helpers.hasProcessFlag("watch=false"),


Why keeping this logic in build-utils.js and repeating it here ?

True, I'll remove it from here

SuperITMan · 2019-02-25T12:51:07Z

packages/stark-build/config/webpack-partial.prod.js

+	PORT: process.env.PORT || 4200,
+	ENV: isProd ? "production" : "development",
+	environment: helpers.hasProcessFlag("e2e") ? "e2e.prod" : "prod",
+	IS_DEV_SERVER: false


Why keeping this logic in build-utils.js and repeating it here ?

carlo-nomes

After a clean:slate I get a whole bunch of errors in my console.
(icon.es5.js:808 Error: <path> attribute d: Expected number, "mdi-ubisoft".) and the icons are not working, I think something with mdi?

I also get this warning in my console I didn't seem to get before.

WARNING in ./src/styles/styles.scss (./node_modules/@angular-devkit/build-angular/src/angular-cli-files/plugins/raw-css-loader.js!./node_modules/postcss-loader/src??embedded!./node_modules/sass-loader/lib/loader.js??ref--14-3!./src/styles/styles.scss)
Module Warning (from ./node_modules/postcss-loader/src/index.js):
Warning

(3726:7) start value has mixed support, consider using flex-start instead
 @ ./src/styles/styles.scss 2:14-250
 @ multi ./node_modules/@nationalbankbelgium/stark-core/assets/css/pre-load-style.css ./src/styles/styles.scss ./src/styles/styles.pcss

carlo-nomes · 2019-02-25T14:23:01Z

docs/stark-build/NG_CLI_BUILD_CUSTOMIZATIONS.md

+                            "plugins": "prepend",
+                            "devServer": "prepend",
+                            "replaceDuplicatePlugins": false
+                        }


Why is prepend needed?

carlo-nomes · 2019-02-25T14:26:51Z

docs/stark-core/GETTING_STARTED.md

+|
+---yourApp
+|   +---assets
+|	|	+---stark-core  // assets from Stark-Core should be available at this location


Is the indentation correct here?

Nop, I'll fix it ;)

carlo-nomes · 2019-02-25T14:29:07Z

docs/stark-ui/GETTING_STARTED.md

+|
+---yourApp
+|   +---assets
+|	|	+---stark-ui  // assets from Stark-UI should be available at this location


Is the indentation correct here?

Nop, I'll fix it ;)

carlo-nomes · 2019-02-25T14:42:51Z

packages/stark-build/config/webpack-partial.common.js

+	// env MONITOR variable set via cross-env
+	if (process.env.MONITOR) {
+		MONITOR = !!process.env.MONITOR;
+	}


Doesn't const MONITOR = !!process.env.MONITOR also work?

True, it is cleaner... I'll fix it ;)

carlo-nomes · 2019-02-25T14:43:44Z

packages/stark-build/config/webpack-partial.common.js

+	// env BUNDLE_ANALYZER variable set via cross-env
+	if (process.env.BUNDLE_ANALYZER) {
+		BUNDLE_ANALYZER = !!process.env.BUNDLE_ANALYZER;
+	}


Doesn't const BUNDLE_ANALYZER = !!process.env.BUNDLE_ANALYZER also work?

True, it is cleaner... I'll fix it ;)

carlo-nomes · 2019-02-25T16:15:00Z

@christophercr, I was wrong it is the update of @mdi/material-icons

christophercr · 2019-02-25T16:57:08Z

@SuperITMan @cnomes I've updated my PR with your remarks. I also fixed the warning about (3726:7) start value has mixed support, consider using flex-start instead .

Can you review this PR again?

christophercr · 2019-02-25T19:06:05Z

@cnomes it seems that they noticed the version 3.5.94 from @mdi/material-icons was buggy so they un-released it :P

#1151
https://travis-ci.org/NationalBankBelgium/stark/builds/498287079
https://www.npmjs.com/package/@mdi/angular-material
Templarian/MaterialDesign-Angular-Material#5

SuperITMan

Still some improvements we could apply 😊

SuperITMan · 2019-02-25T17:57:34Z

starter/package.json

@@ -26,13 +26,15 @@
    "npm": ">=5.6.0"
  },
  "scripts": {
-    "build:aot:prod": "npm run clean:dist && npm run clean:aot && cross-env BUILD_AOT=1 npm run webpack -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.prod.js  --progress --profile --bail",
+    "build:aot:prod": "npm run clean:dist && npm run clean:aot && npm run ng -- build --prod=true --configuration=production",


--prod is enough, you don't need to add =true 😊
Also, you don't need --configuration=production because it is automatically done by Angular CLI when you set --prod.

SuperITMan · 2019-02-25T17:58:56Z

showcase/src/index.html

-		<% for (let i = 0; i < htmlWebpackPlugin.options.dllFiles.css.length; ++i) { %>
-		<link href="<%= htmlWebpackPlugin.options.dllFiles.css[i] %>" rel="stylesheet" />
-		<% } %> <% } %>
+		<!--<% if (htmlWebpackPlugin.options.dllFiles) { %>-->


@christophercr What about this ? 😊

SuperITMan · 2019-02-25T17:59:30Z

showcase/package.json

@@ -26,16 +26,16 @@
    "npm": ">=5.6.0"
  },
  "scripts": {
-    "build:aot:prod": "npm run clean:dist && npm run clean:aot && cross-env BUILD_AOT=1 npm run webpack -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.prod.js  --progress --profile --bail",
-    "build:aot:prod:ci-test-env": "npm run clean:dist && npm run clean:aot && cross-env BUILD_AOT=1 npm run webpack -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.prod.js  --progress --profile --bail --ci-test-env",
+    "build:aot:prod": "npm run clean:dist && npm run clean:aot && npm run ng -- build --prod=true --configuration=production",


--prod is enough, you don't need to add =true 😊
Also, you don't need --configuration=production because it is automatically done by Angular CLI when you set --prod.

SuperITMan · 2019-02-25T18:00:26Z

showcase/config/webpack-custom-config.dev.json

@@ -1,4 +1,4 @@
 {
-  "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io",
+  "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io ws://localhost:4200",


You forgot to remove your change here.
It should be "cspConnectSrc": "http://localhost:5000 http://localhost:5001 http://localhost:5002 http://localhost:4000 https://nationalbankbelgium.github.io",

SuperITMan · 2019-02-25T18:06:40Z

packages/stark-build/config/webpack-partial.dev.js

+const METADATA = Object.assign({}, buildUtils.DEFAULT_METADATA, {
+	HOST: buildUtils.ANGULAR_APP_CONFIG.config.architect.serve.options.host || helpers.getNgCliCommandOption("host") || "localhost", // by default is "localhost"
+	PORT: buildUtils.ANGULAR_APP_CONFIG.config.architect.serve.options.port || helpers.getNgCliCommandOption("port") || 4200, // by default is 4200
+	ENV: isProd ? "production" : "development",


Those variables should be part of buildUtils.DEFAULT_METADATA because they are exactly the same in DEV and in PRO...

ENV: isProd ? "production" : "development", BASE_URL: buildUtils.ANGULAR_APP_CONFIG.buildConfigurations.production.baseHref, HMR: buildUtils.ANGULAR_APP_CONFIG.buildConfigurations.production.hmr || helpers.hasProcessFlag("hmr"), AOT: buildUtils.ANGULAR_APP_CONFIG.buildConfigurations.production.aot || helpers.hasProcessFlag("aot"), WATCH: !(buildUtils.ANGULAR_APP_CONFIG.buildConfigurations.production.watch === false) || !helpers.hasProcessFlag("watch=false"), // by default is true IS_DEV_SERVER: helpers.hasProcessFlag("serve") && !isProd

Nop, they are different.... in DEV they are taken from buildUtils.ANGULAR_APP_CONFIG.buildOptions and in PROD from buildUtils.ANGULAR_APP_CONFIG.buildConfigurations.production

SuperITMan · 2019-02-25T18:25:15Z

showcase/package.json

-    "build:aot:dev": "cross-env BUILD_AOT=1 npm run build:dev",
-    "build:dev": "npm run clean:dist && npm run webpack -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.dev.js --progress --profile",
-    "build:dev:monitor": "npx mkdirp reports && npm run build:dev -- --env.monitor",
+    "build:aot:dev": "npm run build:dev -- --aot=true",


--aot is enough, you don't need to add =true 😊

SuperITMan · 2019-02-25T18:25:46Z

showcase/package.json

-    "server:dev:hmr": "npm run server:dev -- --hot",
-    "server:aot:dev": "cross-env BUILD_AOT=1 npm run server:dev",
-    "server:dev": "npm run clean:dist && npm run webpack-dev-server -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.dev.js --open --progress --profile --watch --content-base src/",
+    "server:dev:hmr": "npm run server:dev -- --hmr=true --configuration=hmr",


For this specifig case, it's a bit different.
According to the official documentation, you can run npm run server:dev -- --configuration hmr.

Of course you have to adapt add a line in your angular.json.

SuperITMan · 2019-02-25T18:26:06Z

showcase/package.json

-    "server:aot:dev": "cross-env BUILD_AOT=1 npm run server:dev",
-    "server:dev": "npm run clean:dist && npm run webpack-dev-server -- --config node_modules/@nationalbankbelgium/stark-build/config/webpack.dev.js --open --progress --profile --watch --content-base src/",
+    "server:dev:hmr": "npm run server:dev -- --hmr=true --configuration=hmr",
+    "server:aot:dev": "npm run server:dev -- --aot=true",


Same here, you can simply put npm run server:dev -- --aot

SuperITMan · 2019-02-25T19:18:33Z

showcase/angular.json

-            "browserTarget": "showcase:build"
+            "browserTarget": "showcase:build",
+            "port": 3000,
+            "open": true
          },
          "configurations": {
            "hmr": {


To make hmr working, please set it like this (according to the official doc):

"hmr": { "hmr": true, "browserTarget": "showcase:build:hmr" }

SuperITMan · 2019-02-25T19:19:00Z

starter/angular.json

-            "browserTarget": "starter:build"
+            "browserTarget": "starter:build",
+            "port": 3000,
+            "open": true
          },
          "configurations": {
            "hmr": {


To make hmr working, please set it like this (according to the official doc):

"hmr": { "hmr": true, "browserTarget": "showcase:build:hmr" }

SuperITMan · 2019-02-26T00:08:55Z

Also, in the commit message, don't forget the mention to BREAKING CHANGE. Otherwise, it won't appear in our CHANGELOG 😕

SuperITMan

Due to a wrong manipulation I have to redo this 😛

christophercr · 2019-02-26T13:18:53Z

@cnomes @SuperITMan I've updated the PR with the latest remarks. Could you please have a look?

I just need to change the commit message with the breaking changes...

christophercr · 2019-02-26T15:16:47Z

And.. I've updated the commit message to add the breaking changes 😉

…r-builders ISSUES CLOSED: #883, #146, #114 BREAKING CHANGE: stark-build now integrates with Angular CLI: - **stark-build:** now provides partial Webpack configurations to override default configuration from Angular CLI. The application's `angular.json` file must be adapted to use this partial configs from stark-build via [@angular-builders](https://github.com/meltedspark/angular-builders). See [Stark-Build: Ng CLI Customizations](./docs/stark-build/NG_CLI_BUILD_CUSTOMIZATIONS.md) - **stark-core:** the application's `angular.json` file must be adapted to add the assets from stark-core to be copied to the application's `assets` folder. See [Stark-Core: Getting Started](./docs/stark-core/GETTING_STARTED.md#assets) - **stark-ui:** the application's `angular.json` file must be adapted to add the assets from stark-ui to be copied to the application's `assets` folder. See [Stark-UI: Getting Started](./docs/stark-ui/GETTING_STARTED.md#assets)

christophercr force-pushed the feature/angular-cli-custom-builders branch 2 times, most recently from f24adbc to 3b5e1dc Compare February 22, 2019 15:07

christophercr added this to the 10.0.0-beta.5 milestone Feb 22, 2019

christophercr added comp: stark-build Stark Build comp: stark-starter Stark Starter comp: stark-showcase type: feature labels Feb 22, 2019

christophercr requested review from SuperITMan and carlo-nomes February 22, 2019 16:15

christophercr force-pushed the feature/angular-cli-custom-builders branch 2 times, most recently from b26b40c to 7b9326e Compare February 22, 2019 17:22

SuperITMan requested changes Feb 25, 2019

View reviewed changes

carlo-nomes reviewed Feb 25, 2019

View reviewed changes

christophercr force-pushed the feature/angular-cli-custom-builders branch 2 times, most recently from 91eeb47 to a03d172 Compare February 25, 2019 16:55

SuperITMan requested changes Feb 25, 2019

View reviewed changes

SuperITMan self-requested a review February 26, 2019 00:09

SuperITMan requested changes Feb 26, 2019

View reviewed changes

christophercr force-pushed the feature/angular-cli-custom-builders branch from a03d172 to b91c616 Compare February 26, 2019 13:16

christophercr force-pushed the feature/angular-cli-custom-builders branch 2 times, most recently from 87c43cf to b97756c Compare February 26, 2019 15:15

christophercr added the breaking change label Feb 26, 2019

christophercr force-pushed the feature/angular-cli-custom-builders branch from b97756c to 9a26c00 Compare February 26, 2019 15:29

SuperITMan approved these changes Feb 26, 2019

View reviewed changes

SuperITMan merged commit 60ea76a into NationalBankBelgium:master Feb 26, 2019

This was referenced Feb 26, 2019

build: bundles design #114

Closed

ng cli support #146

Closed

christophercr deleted the feature/angular-cli-custom-builders branch February 26, 2019 16:53

christophercr mentioned this pull request Mar 1, 2019

showcase: no images displayed in deployed showcase on ghPages due to wrong URL #1036

Closed

christophercr mentioned this pull request Jun 4, 2019

build: integrate jarvis or similar #55

Closed

SuperITMan mentioned this pull request Jun 28, 2023

[Snyk] Fix for 1 vulnerabilities #3616

Closed

SuperITMan mentioned this pull request Jul 8, 2023

[Snyk] Security upgrade @angular-builders/custom-webpack from 12.1.3 to 14.0.0 #3623

Closed

SuperITMan mentioned this pull request Feb 11, 2024

[Snyk] Fix for 1 vulnerabilities #3735

Closed

feat(stark-build): implement integration with Angular CLI via @angular-builders #1148

feat(stark-build): implement integration with Angular CLI via @angular-builders #1148

Conversation

christophercr commented Feb 22, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

christophercr Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophercr Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlo-nomes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlo-nomes Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlo-nomes commented Feb 25, 2019

christophercr commented Feb 25, 2019

christophercr commented Feb 25, 2019 • edited Loading

SuperITMan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperITMan Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperITMan commented Feb 26, 2019

SuperITMan left a comment

Choose a reason for hiding this comment

christophercr commented Feb 26, 2019 • edited Loading

christophercr commented Feb 26, 2019

christophercr commented Feb 22, 2019 •

edited

Loading

coveralls commented Feb 22, 2019 •

edited

Loading

christophercr Feb 25, 2019 •

edited

Loading

christophercr Feb 25, 2019 •

edited

Loading

carlo-nomes left a comment •

edited

Loading

carlo-nomes Feb 25, 2019 •

edited

Loading

christophercr commented Feb 25, 2019 •

edited

Loading

SuperITMan Feb 25, 2019 •

edited

Loading

christophercr commented Feb 26, 2019 •

edited

Loading