Skip to content
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

build: upgrade zone.js #23108

Closed
wants to merge 1 commit into from
Closed

Conversation

JiaLiPassion
Copy link
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
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.

.angulardoc.json Outdated
@@ -0,0 +1,4 @@
{
Copy link
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
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
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
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
Copy link
Contributor

mhevery commented Apr 1, 2018

@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
aio/package.json Outdated
@@ -93,7 +93,7 @@
"rxjs-compat": "6.0.0-beta.4",
"tslib": "^1.9.0",
"web-animations-js": "^2.2.5",
"zone.js": "^0.8.19"
"zone.js": "git+https://github.com/JiaLiPassion/zone.js#newbundle2"
Copy link
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
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

package.json Outdated
@@ -31,7 +31,7 @@
"reflect-metadata": "^0.1.3",
"rxjs": "6.0.0-beta.4",
"tslib": "^1.7.1",
"zone.js": "^0.8.12"
"zone.js": "git+https://github.com/JiaLiPassion/zone.js#newbundle2"
Copy link
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
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
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.

@mhevery
Copy link
Contributor

mhevery commented Apr 3, 2018

@ngbot
Copy link

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
Member

gkalpak commented Apr 4, 2018

🎉

@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.
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.

None yet

6 participants