search

angular / zone.js

Implements Zones for JavaScript
https://github.com/angular/angular/tree/master/packages/zone.js/
MIT License
3.25k stars 407 forks source link

Upgrade this repo and add support to Jasmine 3.3 #1184

Closed IgorMinar closed 5 years ago

IgorMinar commented 5 years ago

Jasmine 3.3 is the latest version of Jasmine and the 3.x release has been out for a while. Many developers, including developers at Google want to use this version, but it's unclear if the zone.js project is compatible with it.

Can we please upgrade the angular/zone.js repo to use jasmine 3.3 and ensure that zone.js supports this version of jasmine. Since there are parts of zone.js that patch jasmine directly, if a change to this patch is required, we'll need to create a jasmine 2.x patch and jasmine 3.x patch or make the patch work for both versions of jasmine (e.g. via conditionally applying parts of the patch depending on the jasmine version) because we shouldn't force people to upgrade the jasmine version just because zone.js updated it's dev dependency.

In any case, we should add at least one simple integration test of the patch against jasmine ^2.9 to ensure that we are not breaking the existing apps.

PS: @johnjbarton has tried to update google to Jasmine 3.3, and got blocked on an issue seemingly coming from zone.js. We haven't been able to create a minimal repro yet, but it's possible that during the jasmine 3.3 upgrade in this repo, we'll see the same issue - so just be aware that the upgrade might not be smooth.

// @JiaLiPassion is this something you could take on please?

JiaLiPassion commented 5 years ago

@IgorMinar, sure, I will take a look @johnjbarton, if you can provide some error information you got, it will be greatly helpful. Thanks.

IgorMinar commented 5 years ago

awesome! thanks @JiaLiPassion

johnjbarton commented 5 years ago

First a couple of hints: our jasmine 3.3 upgrade hit two bumps: the default for tests is now 'random' and the configuration API changed resulting in some mysterious fails. After that we got stuck on fails from code that uses zonejs. I don't know the code involved; the stack I see is:

Chrome 71.0.3578 (Linux 0.0.0): error TypeError: Cannot read property 'apply' of undefined
TypeError: Cannot read property 'apply' of undefined
    at ZoneDelegate.invokeTask (...zone_js/v0_8_21/lib/zone.js:408:31)
    at Zone.runTask (.../zone_js/v0_8_21/lib/zone.js:175:47)
    at drainMicroTaskQueue (.../zone_js/v0_8_21/lib/zone.js:582:35)
JiaLiPassion commented 5 years ago

@johnjbarton, is there any reproduce repo I can access? Thanks!

johnjbarton commented 5 years ago

Sorry, it's not my code, but I passed your request along.

JiaLiPassion commented 5 years ago

@johnjbarton, thanks, the only breaking changes of jasmine 3.3 I knew is nodejs process.unhandledRejection handling, I will also upgrade jasmine to 3.3 in zone.js repo.

And also you may try to set random: false in jasmine config to check whether this issue still there or not.

johnjbarton commented 5 years ago

The code that fails for us currently runs on jasmine 2.9. So the delta we are working with is 3.3-2.9 rather than 3.3-3.2. (We've already set random: false for our 3.3 attempts; also note we are running against your 0.8.21 which I don't control).

Note that the stacktrace I listed above is from code that involves testing errors in async functions.

IgorMinar commented 5 years ago

I suggest we update angular/zone.js, angular/angular, and angular/angular-cli repos to jasmine 3.3 before we try to figure out this google3 specific failure. It's possible that the issue is unrelated to Jasmine 3.3 and zone.js integrations (in google3 we have a project called Catalyst that sits between zone.js and jasmine, so that could be a culprit).

So let's do the updates first, if we come across the issue let's fix it, if not then we have a smaller surface are to deal with to investigate the failure in google3.

On Thu, Feb 7, 2019 at 9:42 AM johnjbarton notifications@github.com wrote:

The code that fails for us currently runs on jasmine 2.9. So the delta we are working with is 3.3-2.9 rather than 3.3-3.2. (We've already set random: false for our 3.3 attempts; also note we are running against your 0.8.21 which I don't control).

Note that the stacktrace I listed above is from code that involves testing errors in async functions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/angular/zone.js/issues/1184#issuecomment-461526626, or mute the thread https://github.com/notifications/unsubscribe-auth/AANM6KBG0JwiwDdHpdNsm2aYaBxe0VlMks5vLGWNgaJpZM4akXZV .

christophercr commented 5 years ago

Question: is #1185 related to this issue? if so, would that mean that Jasmine 3.3 is now supported in angular/zone.js v0.9.0?

JiaLiPassion commented 5 years ago

@christophercr, yes, zone.js 0.9.0 now support jasmine 3.3, if you find any issues, please report here, thanks!

JiaLiPassion commented 5 years ago

I will close this one for now, unless we found new issues for jasmine 3.3