Default sourceMap to empty object #107

joelcogen · 2019-03-25T09:30:35Z

Latest sourceMap option update breaks when we DON'T specific the --sourcemap option.

That's because we set browserOptions.sourceMap to options.sourceMap which is null or undefined.

Error stacktrace:

[ng] Cannot destructure property `styles` of 'undefined' or 'null'.
[ng] TypeError: Cannot destructure property `styles` of 'undefined' or 'null'.
[ng]     at Object.getCommonConfig (/…/node_modules/@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs/common.js:33:107)
[ng]     at BrowserBuilder.buildWebpackConfig (/…/node_modules/@angular-devkit/build-angular/src/browser/index.js:81:31)
[ng]     at CordovaDevServerBuilder.buildWebpackConfig (/…/node_modules/@angular-devkit/build-angular/src/dev-server/index.js:109:46)
[ng]     at CordovaDevServerBuilder.buildWebpackConfig (/…/node_modules/@ionic/angular-toolkit/builders/cordova-serve/index.js:47:22)
[ng]     at MergeMapSubscriber.rxjs_1.from.pipe.operators_1.concatMap [as project] (/…/node_modules/@angular-devkit/build-angular/src/dev-server/index.js:36:40)
[ng]     at MergeMapSubscriber._tryNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:69:27)
[ng]     at MergeMapSubscriber._next (/…/node_modules/rxjs/internal/operators/mergeMap.js:59:18)
[ng]     at MergeMapSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18)
[ng]     at TapSubscriber._next (/…/node_modules/rxjs/internal/operators/tap.js:65:26)
[ng]     at TapSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18)
[ng]     at MergeMapSubscriber.notifyNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:92:26)
[ng]     at InnerSubscriber._next (/…/node_modules/rxjs/internal/InnerSubscriber.js:28:21)
[ng]     at InnerSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18)
[ng]     at MergeMapSubscriber.notifyNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:92:26)
[ng]     at InnerSubscriber._next (/…/node_modules/rxjs/internal/InnerSubscriber.js:28:21)
[ng]     at InnerSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18)

Fixes #109

Latest sourceMap option update breaks when we DON'T specific the `--sourcemap` option. That's because we set `browserOptions.sourceMap` to `options.sourceMap` which is null or undefined. Error stacktrace: ``` [ng] Cannot destructure property `styles` of 'undefined' or 'null'. [ng] TypeError: Cannot destructure property `styles` of 'undefined' or 'null'. [ng] at Object.getCommonConfig (/…/node_modules/@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs/common.js:33:107) [ng] at BrowserBuilder.buildWebpackConfig (/…/node_modules/@angular-devkit/build-angular/src/browser/index.js:81:31) [ng] at CordovaDevServerBuilder.buildWebpackConfig (/…/node_modules/@angular-devkit/build-angular/src/dev-server/index.js:109:46) [ng] at CordovaDevServerBuilder.buildWebpackConfig (/…/node_modules/@ionic/angular-toolkit/builders/cordova-serve/index.js:47:22) [ng] at MergeMapSubscriber.rxjs_1.from.pipe.operators_1.concatMap [as project] (/…/node_modules/@angular-devkit/build-angular/src/dev-server/index.js:36:40) [ng] at MergeMapSubscriber._tryNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:69:27) [ng] at MergeMapSubscriber._next (/…/node_modules/rxjs/internal/operators/mergeMap.js:59:18) [ng] at MergeMapSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18) [ng] at TapSubscriber._next (/…/node_modules/rxjs/internal/operators/tap.js:65:26) [ng] at TapSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18) [ng] at MergeMapSubscriber.notifyNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:92:26) [ng] at InnerSubscriber._next (/…/node_modules/rxjs/internal/InnerSubscriber.js:28:21) [ng] at InnerSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18) [ng] at MergeMapSubscriber.notifyNext (/…/node_modules/rxjs/internal/operators/mergeMap.js:92:26) [ng] at InnerSubscriber._next (/…/node_modules/rxjs/internal/InnerSubscriber.js:28:21) [ng] at InnerSubscriber.Subscriber.next (/…/node_modules/rxjs/internal/Subscriber.js:67:18) ```

imhoffd · 2019-03-25T14:20:43Z

That's odd. What are the reproduction steps? The following command works for me:

❯ npx ng run app:ionic-cordova-build --platform android

Additionally, the schema for the browser builder does not list the option as required: https://github.com/angular/angular-cli/blob/2e9dc3d2528e40ab80a20f3c6f0a3433971fc72d/packages/angular_devkit/build_angular/src/browser/schema.json

joelcogen · 2019-03-26T08:06:21Z

I'm running
ng run app:ionic-cordova-serve:local --host=0.0.0.0 --port=8100 --platform=ios

The way I understand it, the problem is we set sourceMap to null. It is not required, but if the key is present it should be an object. This change also worked for me:

if (options.sourceMap) {
        browserOptions.sourceMap = options.sourceMap;
}

imhoffd · 2019-03-28T18:12:47Z

@joelcogen @carstenbaumhoegger Can I see the configuration in angular.json that this is using?

sourceMap can be undefined, a boolean, or an object with more complicated configuration as specified here: https://github.com/angular/angular-cli/blob/2e9dc3d2528e40ab80a20f3c6f0a3433971fc72d/packages/angular_devkit/build_angular/src/browser/schema.json#L107-L141

I'm guessing null is an unexpected value when it's passed to the Angular builders.

joelcogen · 2019-03-28T20:21:22Z

@dwieeb here's my angular.json

{
  "$schema": "./node_modules/@angular-devkit/core/src/workspace/workspace-schema.json",
  "version": 1,
  "defaultProject": "app",
  "newProjectRoot": "projects",
  "projects": {
    "app": {
      "root": "",
      "sourceRoot": "src",
      "projectType": "application",
      "prefix": "app",
      "schematics": {},
      "architect": {
        "build": {
          "builder": "@angular-devkit/build-angular:browser",
          "options": {
            "outputPath": "www",
            "index": "src/index.html",
            "main": "src/main.ts",
            "polyfills": "src/polyfills.ts",
            "tsConfig": "src/tsconfig.app.json",
            "assets": [
              {
                "glob": "**/*",
                "input": "src/assets",
                "output": "assets"
              },
              {
                "glob": "**/*.svg",
                "input": "node_modules/ionicons/dist/ionicons/svg",
                "output": "./svg"
              }
            ],
            "styles": [
              {
                "input": "src/theme/variables.scss"
              },
              {
                "input": "src/global.scss"
              }
            ],
            "scripts": []
          },
          "configurations": {
            "local": {
              "fileReplacements": [
                {
                  "replace": "src/environments/environment.ts",
                  "with": "src/environments/environment.local.ts"
                }
              ]
            },
            "dev": {
              "fileReplacements": [
                {
                  "replace": "src/environments/environment.ts",
                  "with": "src/environments/environment.dev.ts"
                }
              ]
            },
            "staging": {
              "optimization": true,
              "outputHashing": "all",
              "sourceMap": false,
              "extractCss": true,
              "namedChunks": false,
              "aot": true,
              "extractLicenses": true,
              "vendorChunk": false,
              "buildOptimizer": true,
              "fileReplacements": [
                {
                  "replace": "src/environments/environment.ts",
                  "with": "src/environments/environment.staging.ts"
                }
              ]
            },
            "production": {
              "fileReplacements": [
                {
                  "replace": "src/environments/environment.ts",
                  "with": "src/environments/environment.production.ts"
                }
              ],
              "optimization": true,
              "outputHashing": "all",
              "sourceMap": false,
              "extractCss": true,
              "namedChunks": false,
              "aot": true,
              "extractLicenses": true,
              "vendorChunk": false,
              "buildOptimizer": true,
              "budgets": [
                {
                  "type": "initial",
                  "maximumWarning": "2mb",
                  "maximumError": "5mb"
                }
              ]
            },
            "ci": {
              "progress": false
            }
          }
        },
        "serve": {
          "builder": "@angular-devkit/build-angular:dev-server",
          "options": {
            "browserTarget": "app:build",
            "proxyConfig": "proxy.conf.json"
          },
          "configurations": {
            "local": {
              "browserTarget": "app:build:local"
            },
            "dev": {
              "browserTarget": "app:build:dev"
            },
            "test": {
              "browserTarget": "app:build:test"
            },
            "staging": {
              "browserTarget": "app:build:staging"
            },
            "production": {
              "browserTarget": "app:build:production"
            },
            "ci": {
              "progress": false
            }
          }
        },
        "extract-i18n": {
          "builder": "@angular-devkit/build-angular:extract-i18n",
          "options": {
            "browserTarget": "app:build"
          }
        },
        "test": {
          "builder": "@angular-devkit/build-angular:karma",
          "options": {
            "main": "src/test.ts",
            "polyfills": "src/polyfills.ts",
            "tsConfig": "src/tsconfig.spec.json",
            "karmaConfig": "src/karma.conf.js",
            "styles": [],
            "scripts": [],
            "assets": [
              {
                "glob": "favicon.ico",
                "input": "src/",
                "output": "/"
              },
              {
                "glob": "**/*",
                "input": "src/assets",
                "output": "/assets"
              }
            ]
          },
          "configurations": {
            "ci": {
              "progress": false,
              "watch": false
            }
          }
        },
        "lint": {
          "builder": "@angular-devkit/build-angular:tslint",
          "options": {
            "tsConfig": ["src/tsconfig.app.json", "src/tsconfig.spec.json"],
            "exclude": ["**/node_modules/**"]
          }
        },
        "ionic-cordova-build": {
          "builder": "@ionic/angular-toolkit:cordova-build",
          "options": {
            "browserTarget": "app:build"
          },
          "configurations": {
            "local": {
              "browserTarget": "app:build:local"
            },
            "dev": {
              "browserTarget": "app:build:dev"
            },
            "test": {
              "browserTarget": "app:build:test"
            },
            "staging": {
              "browserTarget": "app:build:staging"
            },
            "production": {
              "browserTarget": "app:build:production"
            }
          }
        },
        "ionic-cordova-serve": {
          "builder": "@ionic/angular-toolkit:cordova-serve",
          "options": {
            "cordovaBuildTarget": "app:ionic-cordova-build",
            "devServerTarget": "app:serve"
          },
          "configurations": {
            "local": {
              "cordovaBuildTarget": "app:ionic-cordova-build:local",
              "devServerTarget": "app:serve:local"
            },
            "dev": {
              "cordovaBuildTarget": "app:ionic-cordova-build:dev",
              "devServerTarget": "app:serve:dev"
            },
            "test": {
              "cordovaBuildTarget": "app:ionic-cordova-build:test",
              "devServerTarget": "app:serve:test"
            },
            "staging": {
              "cordovaBuildTarget": "app:ionic-cordova-build:staging",
              "devServerTarget": "app:serve:staging"
            },
            "production": {
              "cordovaBuildTarget": "app:ionic-cordova-build:production",
              "devServerTarget": "app:serve:production"
            }
          }
        }
      }
    },
    "app-e2e": {
      "root": "e2e/",
      "projectType": "application",
      "architect": {
        "e2e": {
          "builder": "@angular-devkit/build-angular:protractor",
          "options": {
            "protractorConfig": "e2e/protractor.conf.js",
            "devServerTarget": "app:serve"
          },
          "configurations": {
            "ci": {
              "devServerTarget": "app:serve:ci"
            }
          }
        },
        "lint": {
          "builder": "@angular-devkit/build-angular:tslint",
          "options": {
            "tsConfig": "e2e/tsconfig.e2e.json",
            "exclude": ["**/node_modules/**"]
          }
        }
      }
    }
  },
  "cli": {
    "defaultCollection": "@ionic/angular-toolkit"
  },
  "schematics": {
    "@ionic/angular-toolkit:component": {
      "styleext": "scss"
    },
    "@ionic/angular-toolkit:page": {
      "styleext": "scss"
    }
  }
}

imhoffd · 2019-03-28T21:07:31Z

Interesting. This is actually due to that use of as any. The error is coming from here: https://github.com/angular/angular-cli/blob/2f791395d2d416fbe1a9201dde5a1cd2449393dc/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts#L37-L41

Without the as any, TypeScript is giving the following compile error:

builders/cordova-build/index.ts:50:5 - error TS2322: Type 'boolean | undefined' is not assignable to type 'SourceMapOptions'.
  Type 'undefined' is not assignable to type 'SourceMapOptions'.

50     browserOptions.sourceMap = options.sourceMap;

I also found out something new about object destructuring:

> const { foo } = undefined
TypeError: Cannot destructure property `foo` of 'undefined' or 'null'.
> const { bar } = false
undefined

imhoffd · 2019-03-28T21:15:57Z

I think a proper fix for this would be something similar to the code in your comment.

If the sourceMap option is specified, overwrite browserOptions.sourceMap. Otherwise, don't.

However, we need to check if it's truly specified (and not miss the false use case). So maybe something like this:

if (typeof options.sourceMap !== 'undefined') {
  browserOptions.sourceMap = options.sourceMap;
}

carstenbaumhoegger · 2019-03-29T07:46:02Z

@dwieeb this is my angular.json:

{
  "$schema": "./node_modules/@angular-devkit/core/src/workspace/workspace-schema.json",
  "version": 1,
  "projects": {
    "app": {
      "root": "",
      "projectType": "application",
      "architect": {
        "build": {
          "builder": "@angular-devkit/build-angular:browser",
          "options": {
            "progress": false,
            "outputPath": "www",
            "index": "src/index.html",
            "main": "src/main.ts",
            "polyfills": "src/polyfills.ts",
            "tsConfig": "src/tsconfig.app.json",
            "assets": [
              {
                "glob": "**/*",
                "input": "src/assets",
                "output": "assets"
              },
              {
                "glob": "**/*.svg",
                "input": "node_modules/ionicons/dist/ionicons/svg",
                "output": "./svg"
              }
            ],
            "styles": [
              {
                "input": "src/theme/variables.scss"
              },
              {
                "input": "src/global.scss"
              }
            ],
            "scripts": []
          },
          "configurations": {
            "production": {
              "fileReplacements": [
                {
                  "replace": "src/environments/environment.ts",
                  "with": "src/environments/environment.prod.ts"
                }
              ],
              "optimization": true,
              "outputHashing": "all",
              "sourceMap": false,
              "extractCss": true,
              "namedChunks": false,
              "aot": true,
              "extractLicenses": true,
              "vendorChunk": false,
              "buildOptimizer": true
            }
          }
        },
        "serve": {
          "builder": "@angular-devkit/build-angular:dev-server",
          "options": {
            "browserTarget": "app:build"
          },
          "configurations": {
            "production": {
              "browserTarget": "app:build:production"
            }
          }
        },
        "extract-i18n": {
          "builder": "@angular-devkit/build-angular:extract-i18n",
          "options": {
            "browserTarget": "app:build"
          }
        },
        "test": {
          "builder": "@angular-devkit/build-angular:karma",
          "options": {
            "main": "src/test.ts",
            "polyfills": "src/polyfills.ts",
            "tsConfig": "src/tsconfig.spec.json",
            "karmaConfig": "src/karma.conf.js",
            "styles": [
              {
                "input": "styles.css"
              }
            ],
            "scripts": [],
            "assets": [
              {
                "glob": "favicon.ico",
                "input": "src/",
                "output": "/"
              },
              {
                "glob": "**/*",
                "input": "src/assets",
                "output": "/assets"
              }
            ]
          }
        },
        "lint": {
          "builder": "@angular-devkit/build-angular:tslint",
          "options": {
            "tsConfig": [
              "src/tsconfig.app.json",
              "src/tsconfig.spec.json"
            ],
            "exclude": [
              "**/node_modules/**"
            ]
          }
        },
        "ionic-cordova-build": {
          "builder": "@ionic/angular-toolkit:cordova-build",
          "options": {
            "browserTarget": "app:build"
          },
          "configurations": {
            "production": {
              "browserTarget": "app:build:production"
            }
          }
        },
        "ionic-cordova-serve": {
          "builder": "@ionic/angular-toolkit:cordova-serve",
          "options": {
            "cordovaBuildTarget": "app:ionic-cordova-build",
            "devServerTarget": "app:serve"
          },
          "configurations": {
            "production": {
              "cordovaBuildTarget": "app:ionic-cordova-build:production",
              "devServerTarget": "app:serve:production"
            }
          }
        }
      }
    },
    "app-e2e": {
      "root": "e2e/",
      "projectType": "application",
      "architect": {
        "e2e": {
          "builder": "@angular-devkit/build-angular:protractor",
          "options": {
            "protractorConfig": "e2e/protractor.conf.js",
            "devServerTarget": "app:serve",
            "port": 4202
          }
        },
        "lint": {
          "builder": "@angular-devkit/build-angular:tslint",
          "options": {
            "tsConfig": "e2e/tsconfig.e2e.json",
            "exclude": [
              "**/node_modules/**"
            ]
          }
        }
      }
    }
  },
  "cli": {
    "defaultCollection": "@ionic/schematics-angular"
  },
  "schematics": {
    "@ionic/schematics-angular:component": {
      "styleext": "scss"
    },
    "@ionic/schematics-angular:page": {
      "styleext": "scss"
    }
  },
  "defaultProject": "app"
}

imhoffd · 2019-04-04T16:31:53Z

@joelcogen Thoughts on my comment?

jmesa-sistel · 2019-04-05T14:01:31Z

I think a proper fix for this would be something similar to the code in your comment.

If the sourceMap option is specified, overwrite browserOptions.sourceMap. Otherwise, don't.

However, we need to check if it's truly specified (and not miss the false use case). So maybe something like this:
if (typeof options.sourceMap !== 'undefined') {
  browserOptions.sourceMap = options.sourceMap;
}

Maybe ?

if (options.sourceMap != null) {
  browserOptions.sourceMap = options.sourceMap;
}

joelcogen · 2019-04-08T07:53:59Z

@dwieeb Updated accordingly, wdyt?

jmesa-sistel · 2019-04-08T10:01:35Z

@dwieeb Updated accordingly, wdyt?

I guess you mean
if (typeof options.sourceMap !== 'undefined' && options.sourceMap !== null)
Because the != operator checks for both, undefined and null
So I think if "better" or more clear to use:
if (options.sourceMap != null)
Just my 2 cents

imhoffd · 2019-04-08T14:30:42Z

builders/cordova-build/schema.json will restrict which types are allowed in. options.sourceMap can never be null. You can test this by passing in bad values:

        "ionic-cordova-build": {
          "builder": "@ionic/angular-toolkit:cordova-build",
          "options": {
            "browserTarget": "app:build",
            "sourceMap": null
          },

Its type is correct (undefined | boolean):

We're just incorrectly coercing it to any.

I think the linter doesn't like the options.sourceMap != null part because indeed, it's always true.

joelcogen · 2019-04-09T08:32:56Z

I guess this change is enough then?

imhoffd · 2019-04-09T16:16:14Z

I think so! I'll get this merged in and released.

## [1.5.1](v1.5.0...v1.5.1) (2019-04-09) ### Bug Fixes * **cordova-build:** only set sourceMap if specified ([#107](#107)) ([2a99ac0](2a99ac0))

Ionitron · 2019-04-09T16:25:06Z

🎉 This PR is included in version 1.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

## [1.5.1](ionic-team/angular-toolkit@v1.5.0...v1.5.1) (2019-04-09) ### Bug Fixes * **cordova-build:** only set sourceMap if specified ([#107](ionic-team/angular-toolkit#107)) ([2a99ac0](ionic-team/angular-toolkit@2a99ac0))

imhoffd mentioned this pull request Apr 4, 2019

Cannot destructure property styles of 'undefined' or 'null'. #109

Closed

Don't assign undefined or null to browserOptions.sourceMap

5170cfb

options.sourceMap can never be null

eaa3d13

imhoffd approved these changes Apr 9, 2019

View reviewed changes

imhoffd merged commit 2a99ac0 into ionic-team:master Apr 9, 2019

Ionitron added a commit that referenced this pull request Apr 9, 2019

chore(release): 1.5.1 [skip ci]

0cf9e9c

## [1.5.1](v1.5.0...v1.5.1) (2019-04-09) ### Bug Fixes * **cordova-build:** only set sourceMap if specified ([#107](#107)) ([2a99ac0](2a99ac0))

Ionitron added the released label Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default sourceMap to empty object #107

Default sourceMap to empty object #107

joelcogen commented Mar 25, 2019 •

edited by imhoffd

Loading

imhoffd commented Mar 25, 2019

joelcogen commented Mar 26, 2019

imhoffd commented Mar 28, 2019

joelcogen commented Mar 28, 2019

imhoffd commented Mar 28, 2019 •

edited

Loading

imhoffd commented Mar 28, 2019 •

edited

Loading

carstenbaumhoegger commented Mar 29, 2019

imhoffd commented Apr 4, 2019

jmesa-sistel commented Apr 5, 2019

joelcogen commented Apr 8, 2019

jmesa-sistel commented Apr 8, 2019

imhoffd commented Apr 8, 2019

joelcogen commented Apr 9, 2019

imhoffd commented Apr 9, 2019

Ionitron commented Apr 9, 2019

Default sourceMap to empty object #107

Default sourceMap to empty object #107

Conversation

joelcogen commented Mar 25, 2019 • edited by imhoffd Loading

imhoffd commented Mar 25, 2019

joelcogen commented Mar 26, 2019

imhoffd commented Mar 28, 2019

joelcogen commented Mar 28, 2019

imhoffd commented Mar 28, 2019 • edited Loading

imhoffd commented Mar 28, 2019 • edited Loading

carstenbaumhoegger commented Mar 29, 2019

imhoffd commented Apr 4, 2019

jmesa-sistel commented Apr 5, 2019

joelcogen commented Apr 8, 2019

jmesa-sistel commented Apr 8, 2019

imhoffd commented Apr 8, 2019

joelcogen commented Apr 9, 2019

imhoffd commented Apr 9, 2019

Ionitron commented Apr 9, 2019

joelcogen commented Mar 25, 2019 •

edited by imhoffd

Loading

imhoffd commented Mar 28, 2019 •

edited

Loading

imhoffd commented Mar 28, 2019 •

edited

Loading