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.

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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@benbraou
Copy link
Contributor

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

@googlebot googlebot added the cla: yes label Feb 6, 2018

@@ -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');

This comment has been minimized.

@benbraou

benbraou Feb 6, 2018

Author Contributor

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 benbraou:issue22027 branch 2 times, most recently from 889d64b to 65ef710 Feb 6, 2018

@benbraou

This comment has been minimized.

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 force-pushed the benbraou:issue22027 branch from 65ef710 to 5ee92be Feb 6, 2018

@benbraou benbraou referenced this pull request Feb 6, 2018

Closed

feat(core): turn whitespace removal on by default #22050

2 of 3 tasks complete
@mhevery

mhevery approved these changes Feb 6, 2018

@mhevery mhevery self-assigned this Feb 6, 2018

@mhevery

This comment has been minimized.

@mhevery mhevery requested a review from IgorMinar Feb 6, 2018

@IgorMinar
Copy link
Member

IgorMinar left a comment

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

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.

This comment has been minimized.

@IgorMinar

IgorMinar Feb 7, 2018

Member

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

This comment has been minimized.

@benbraou

benbraou Feb 7, 2018

Author Contributor

@IgorMinar done.

@benbraou benbraou force-pushed the benbraou:issue22027 branch from 5ee92be to 0eb0639 Feb 7, 2018

@benbraou

This comment has been minimized.

Copy link
Contributor Author

benbraou commented Feb 7, 2018

aot-compiler.md updated as requested and commit amended

@ngbot

This comment has been minimized.

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!

@benbraou

This comment has been minimized.

Copy link
Contributor Author

benbraou commented Feb 13, 2018

Merge conflict in aot-compiler.md fixed

@benbraou benbraou force-pushed the benbraou:issue22027 branch from e2023d3 to 6135515 Feb 13, 2018

@benbraou

This comment has been minimized.

Copy link
Contributor Author

benbraou commented Feb 13, 2018

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

This comment has been minimized.

Copy link
Member

IgorMinar commented Feb 13, 2018

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

@benbraou

This comment has been minimized.

Copy link
Contributor Author

benbraou commented Feb 13, 2018

The CI build is now passing. Thank you!

@mhevery mhevery added this to Sprint Backlog in Framework via automation Feb 14, 2018

@mhevery mhevery moved this from Sprint Backlog to In progress in Framework Feb 14, 2018

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Feb 15, 2018

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

@vicb vicb closed this in f1a0632 Feb 16, 2018

Framework automation moved this from In progress to Done / Merged Feb 16, 2018

@benbraou benbraou deleted the benbraou:issue22027 branch Feb 17, 2018

jbogarthyde added a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

@mhevery mhevery removed this from Done / Merged in Framework Feb 28, 2018

@ahnpnl

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

IgorMinar commented Mar 2, 2018

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

@ahnpnl

This comment has been minimized.

Copy link

ahnpnl commented Mar 2, 2018

hi @IgorMinar, it worked :) Nothing strange happens

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Mar 2, 2018

cool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment