Skip to content

build: upgrade zone.js#23108

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:zone821
Closed

build: upgrade zone.js#23108
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:zone821

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

based on #23100
Upgrade zone.js to latest version, currently this PR is just for test, because it depends on my zone.js branch.
in zone.js 0.8.21, there are several feature which break angular test.

  1. move async/fakeAsync from angular to zone.js
  2. support jasmine.clock() to be auto jump into fakeAsync
beforeEach(() => {
      jasmine.clock().install();
    });

    afterEach(() => {
      jasmine.clock().uninstall();
    });

    it('should get date diff correctly', () => {  // we don't need fakeAsync here.
      // automatically run into fake async zone, because jasmine.clock() is installed.
      const start = Date.now();
      jasmine.clock().tick(100);
      const end = Date.now();
      expect(end - start).toBe(100);
    });

this feature break the test cases in aio. https://github.com/angular/angular/blob/master/aio/src/app/app.component.spec.ts#L1110
which use jasmine.clock() to simulate timer, but still use async/await, so it will be TIMEOUT, in this PR, just remove async/await and use tick.

  1. several test cases in angular will be timeout, https://travis-ci.org/angular/angular/jobs/360448001
    the reason is because zone.js AsyncTestZoneSpec add a new feature to be able to wait non resolved promise.then, it seems this feature will have a big impact to current angular test cases, for example, in the following case,
    https://github.com/angular/angular/blob/master/packages/compiler/test/metadata_resolver_spec.ts#L87
    current test case seems have some error, that the promise.then never invoked, but in earlier version of zone.js, this test case will pass, but in zone.js 0.8.21, this will fail. there are several similar cases, so I just disable this feature in fix(fakeAsync): fix #1050, should only reset patched Date.now until fakeAsync exit  zone.js#1051, and I will check all related test cases in angular later.

@JiaLiPassion JiaLiPassion force-pushed the zone821 branch 14 times, most recently from 1e57798 to 2f60702 Compare March 31, 2018 13:51
@JiaLiPassion JiaLiPassion changed the title WIP: upgrade zone.js build: upgrade zone.js Mar 31, 2018
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

There seem to be a lot of unrelated formatting changes in the source code. Could you revert those?

Thank you for looking into this.

Comment thread .angulardoc.json Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this file be reverted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mhevery , yes, I will revert it now.

@JiaLiPassion JiaLiPassion force-pushed the zone821 branch 6 times, most recently from edebbe2 to d7fd35f Compare April 1, 2018 02:21
Copy link
Copy Markdown
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.

Is this a breaking change in zone.js that should be documented and require a minor version bump?

In other words do we expect others to have to make these changes as well? Should we update our testing docs?

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@IgorMinar , I will update the test document now, about the break change such as user don't need to write fakeAsync when using jasmine.clock, I am not sure where I should document it. should it be documented in change log?

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Apr 1, 2018
@JiaLiPassion JiaLiPassion force-pushed the zone821 branch 4 times, most recently from af65100 to b6c63cc Compare April 3, 2018 06:37
@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Apr 3, 2018
gkalpak
gkalpak previously requested changes Apr 3, 2018
Comment thread aio/package.json Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be replaced to point to an npm release?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak , sure, if the g3 test passed, zone.js will have a new release

Comment thread package.json Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be replaced to point to an npm release?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same as above.

@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Apr 3, 2018

Removed the merge label because of #23108 (comment).
@mhevery, feel free to add it back if it was intentional to use @JiaLiPassion's branch.

@ngbot
Copy link
Copy Markdown

ngbot Bot commented Apr 3, 2018

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

@JiaLiPassion JiaLiPassion force-pushed the zone821 branch 4 times, most recently from 439eb99 to eea5c5b Compare April 4, 2018 01:26
@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 Apr 4, 2018
@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Apr 4, 2018

🎉

@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants