Skip to content

feat(core): set preserveWhitespaces to false by default #22046

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat(core): set preserveWhitespaces to false by default #22046

wants to merge 1 commit into from

Conversation

benbraou
Copy link
Contributor

@benbraou benbraou commented Feb 6, 2018

Fixes #22027

PR Checklist

Please check if your PR fulfills the following requirements:

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)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

preserveWhitespaces is true by default

What is the new behavior?

preserveWhitespaces is false by default

Does this PR introduce a breaking change?

[x] Yes
[ ] No

The impact of this breaking change seems to be already assessed as minimal as described in #22027

Other information

benbraou
benbraou commented Feb 6, 2018
View reviewed changes
packages/platform-browser/test/testing_public_spec.ts
@@ -307,7 +307,7 @@ class CompWithUrlTemplate {
it('should allow to createSync components with templateUrl after explicit async compilation',
() => {
const fixture = TestBed.createComponent(CompWithUrlTemplate);
expect(fixture.nativeElement).toHaveText('from external template\n');
expect(fixture.nativeElement).toHaveText('from external template');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here (as well as in other tests) I have just updated the expected value rather then having two tests: one with preserveWhiteSpaces=true and preserveWhiteSpaces=false. I did not add the extra test because this does not seem to be the purpose of the existing test (createSync components).

@benbraou benbraou force-pushed the issue22027 branch 2 times, most recently from 889d64b to 65ef710 Compare February 6, 2018 20:37
@benbraou
Copy link
Contributor Author

benbraou commented Feb 6, 2018

I get the error Commit 437aced2ab3cffa63ffa0707214d9b870b08462a uncompressed main exceeded expected size by >1% (expected: 459119, actual: 464506).
This is because I have removed from aio/src/tsconfig.app.json preserveWhitespaces setting. The aio project depends on angular 5.2.0 and not a the built version(where preserveWhitespaces is false by default). That's why, I will re-set aio/src/tsconfig.app.json. We may want to remove the option when aio migrates to Angular 6

@benbraou benbraou mentioned this pull request Feb 6, 2018
Closed
3 tasks
@mhevery mhevery self-assigned this Feb 6, 2018
@mhevery mhevery added the target: major This PR is targeted for the next major release label Feb 6, 2018
@mhevery
Copy link
Contributor

mhevery commented Feb 6, 2018

http://test/OCL:184739848:BASE:184740273:1517956003963:61235fa8

@mhevery mhevery requested a review from IgorMinar February 6, 2018 22:27
IgorMinar
IgorMinar approved these changes Feb 7, 2018
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

can you just change that one doc please? the rest looks great. thanks!!

aio/content/guide/aot-compiler.md Outdated
This option is `true` by default.

*Note*: It is recommended to set this explicitly to `false` as it emits smaller template factory modules and might be set to `false` by default in the future.
This option is `false` by default, which results in smaller emitted template factory modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe say, "As of v6, this option is false by default ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IgorMinar done.

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 7, 2018
@benbraou
Copy link
Contributor Author

benbraou commented Feb 7, 2018 •
edited
Loading

aot-compiler.md updated as requested and commit amended

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 7, 2018
@mhevery
Copy link
Contributor

mhevery commented Feb 9, 2018 •
edited
Loading

http://test/OCL:184739848:BASE:185183195:1518211077966:cc147f29
global: https://test.corp.google.com/OCL:184739848:BASE:185183569:1518211250577:292458cc

@ngbot
Copy link

ngbot bot commented Feb 13, 2018

Hi @benbraou! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@mhevery mhevery added the area: core Issues related to the framework runtime label Feb 13, 2018
@benbraou
Copy link
Contributor Author

Merge conflict in aot-compiler.md fixed

@benbraou
Copy link
Contributor Author

Weird... After rebase on top of master, the build job CI=ai_tools_test is failing (locally, aio tests are passing)

I see the error

Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL

I am not sure if just a rebuild is needed or a more serious problem is causing this.
The change in aot-compiler.md is pretty simple.
I will look at this tomorrow. Meanwhile, any hint would be great 😃

@IgorMinar
Copy link
Contributor

I've restarted the ci job. let's see if we can repro the issue again.

@benbraou
Copy link
Contributor Author

The CI build is now passing. Thank you!

@vicb vicb added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 15, 2018
@IgorMinar
Copy link
Contributor

@mhevery I thought you tested this in g3 already? can we land it?

@vicb vicb removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Feb 16, 2018
@vicb vicb closed this in f1a0632 Feb 16, 2018
@benbraou benbraou deleted the issue22027 branch February 17, 2018 18:53
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
@benbraou @jbogarthyde
feat(core): set preserveWhitespaces to false by default (angular#22046)
73850d4 
Fixes angular#22027

PR Close angular#22046
smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018
@benbraou @smdunn809
feat(core): set preserveWhitespaces to false by default (angular#22046)
f58a08f 
Fixes angular#22027

PR Close angular#22046
@ahnpnl
Copy link
Contributor

ahnpnl commented Mar 1, 2018

hi @mhevery, I have a question regarding to this. Does TestBed also automatically get this option false by default ?

@IgorMinar
Copy link
Contributor

IgorMinar commented Mar 2, 2018 •
edited
Loading

@AhnpGit, it should. do you see something else? if so, please file an issue and provide a reproduction.

@ahnpnl
Copy link
Contributor

ahnpnl commented Mar 2, 2018

hi @IgorMinar, it worked :) Nothing strange happens

@IgorMinar
Copy link
Contributor

cool

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@benbraou @leo6104
feat(core): set preserveWhitespaces to false by default (angular#22046)
07bb45a 
Fixes angular#22027

PR Close angular#22046
@pfeigl pfeigl mentioned this pull request May 19, 2018
Closed
@biiiipy biiiipy mentioned this pull request May 23, 2018
Closed
@jenniferfell jenniferfell mentioned this pull request Jun 6, 2018
Closed
jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018
@benbraou @vicb
feat(core): set preserveWhitespaces to false by default (angular#22046)
3142add 
Fixes angular#22027

PR Close angular#22046
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@IgorMinar IgorMinar IgorMinar approved these changes

Assignees

@mhevery mhevery

Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimize whitespace in template by default (preserveWhitespace to false by default)
6 participants
@benbraou @mhevery @IgorMinar @ahnpnl @vicb @googlebot