refactor(build): consolidate build options by filipesilva · Pull Request #4105 · angular/angular-cli

filipesilva · 2017-01-19T00:31:39Z

BREAKING CHANGE:

--extractCss defaults to false on all --dev (ng build with no flags uses --dev)
--aot defaults to true in --prod
the alias for --output-path is now -op instead of -o

hansl · 2017-01-19T01:08:59Z

Could you move XXX.run.ts to a separate directory? Seems like ng help still shows those.

filipesilva · 2017-01-19T12:43:20Z

@hansl I'll do that in a followup PR. There's a few followups to this one that I want to do, but trying to keep the scope of each down.

changLiuUNSW · 2017-01-19T13:11:45Z

packages/angular-cli/tasks/build-webpack.ts

    const project = this.cliProject;

-    const outputDir = runTaskOptions.outputPath || CliConfig.fromProject().config.apps[0].outDir;
-    const deployUrl = runTaskOptions.deployUrl ||


seemed you remove the functionality of using deployUrl via the config file

Sorry.
I just find it in addAppConfigDefaults

I transferred it over to packages/angular-cli/models/webpack-config.ts, in the addAppConfigDefaults function.

To make sure your functionality in #4090 also is in, I force a value for deployUrl in packages/angular-cli/tasks/serve-webpack.ts via serveDefaults.

This way in for ng serve, the default for --deploy-url is '' instead of undefined and thus the default in angular-cli.json is not used.

I believe this was your intention.

Yes, that is my intention.
Did you test the css url path whether it points to the correct place with --extractCss=false and --deploy-url?
I suspect it may have same issue like:
#4035

I refactored the tests around a bit and that test is in tests/e2e/tests/build/styles/extract-css.ts, the last test. Can you double check if that's right?

changLiuUNSW · 2017-01-19T13:46:44Z

tests/e2e/tests/build/styles/extract-css.ts

+      <script type="text/javascript" src="main.bundle.js"></script>
+    `))
+    // verify --deploy-url isn't applied to extracted urls
    .then(() => ng('build', '--extract-css', '--deploy-url=client/'))


Can we add a test with --deploy-url=client/ and --extract-css=false?

What should be the output?

I think if --extract-css=false, styles will be inserted as <style> tags into the index.html. Normally the index.html will not be under the same path with deploy-url , so the CSS asset url should point to the full path [deployUrl]/[assetName]. In this case, the correct result should be client/more.svg
However, I cannot make sure now. I will have a check tomorrow.

@filipesilva
Sorry for keep editing the comment....
Not sure what the expect result right now. Will update after verifying later

Ok let me know when you figure it out. Worst case scenario the new test goes into a new PR.

I believe there should be a way --deployUrl/ also work with ng serve (it's mentioned here) but haven't looked into it much as it's out of scope of this PR.

Confirmed.
Should be the full path [deployUrl]/[assetName]. In this case, it is client/more.svg.
Do not how you test it, because the CSS is inserted by JavaScript when page is loaded

filipesilva · 2017-01-20T21:29:19Z

@changLiuUNSW added this test to address #4105 (comment)

    // verify --deploy-url is applied to non-extracted css urls
    .then(() => ng('build', '--deploy-url=deployUrl/', '--extract-css=false'))
    .then(() => expectFileToMatch('dist/styles.bundle.css', '__webpack_require__.p \+ \"more.svg\"'));

Edit: updated the test, turns out the import looks like this instead: __webpack_require__.p + "more.svg";

changLiuUNSW · 2017-01-21T13:54:04Z

LGTM

Brocco · 2017-01-24T15:46:14Z

Please update documentation to account for changes in options. For example output path's alias has gone from -o to -op

Fix angular#4138 BREAKING CHANGE: - `--extractCss` defaults to `false` on all `--dev` (`ng build` with no flags uses `--dev`) - `--aot` defaults to true in `--prod` - the alias for `--output-path` is now `-op` instead of `-o`

angular-automatic-lock-bot · 2019-09-11T19:41:40Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jan 19, 2017

filipesilva force-pushed the refactor-build-options branch 2 times, most recently from e657f2c to c93ffca Compare January 19, 2017 00:59

filipesilva mentioned this pull request Jan 19, 2017

refactor (build): refactor the logic to set publicPath #4090

Closed

changLiuUNSW reviewed Jan 19, 2017

View reviewed changes

filipesilva force-pushed the refactor-build-options branch from c93ffca to dedf216 Compare January 19, 2017 13:34

changLiuUNSW reviewed Jan 19, 2017

View reviewed changes

filipesilva added the pr_action: review label Jan 19, 2017

filipesilva force-pushed the refactor-build-options branch 2 times, most recently from a6896a0 to 90c9dd1 Compare January 19, 2017 22:39

filipesilva force-pushed the refactor-build-options branch from 8746401 to dab74e8 Compare January 20, 2017 22:28

filipesilva force-pushed the refactor-build-options branch from d339bb1 to dfa54da Compare January 22, 2017 20:17

Brocco approved these changes Jan 24, 2017

View reviewed changes

filipesilva added pr_action: merge and removed pr_action: review labels Jan 24, 2017

filipesilva added 7 commits January 24, 2017 17:07

refactor(build): consolidate build options

9dee17d

correctly match main bundle

db656b2

fix package sort

bb7a444

fix extract test

b780224

change output path flag in docs

6401b2c

fix script order again arghhhh

abddb86

correct build and serve options

62f8f98

filipesilva force-pushed the refactor-build-options branch from bbc9e53 to 62f8f98 Compare January 24, 2017 17:16

filipesilva merged commit e15433e into angular:master Jan 24, 2017

filipesilva deleted the refactor-build-options branch January 24, 2017 17:51

filipesilva mentioned this pull request Jan 27, 2017

refactor(commands): turn .run commands into tasks #4251

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

Conversation

filipesilva commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansl commented Jan 19, 2017

Uh oh!

filipesilva commented Jan 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changLiuUNSW Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changLiuUNSW Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

changLiuUNSW Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filipesilva commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changLiuUNSW commented Jan 21, 2017

Uh oh!

Brocco commented Jan 24, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

filipesilva commented Jan 19, 2017 •

edited

Loading

changLiuUNSW Jan 19, 2017 •

edited

Loading

changLiuUNSW Jan 19, 2017 •

edited

Loading

changLiuUNSW Jan 20, 2017 •

edited

Loading

filipesilva commented Jan 20, 2017 •

edited

Loading