Add a nativescript zone implementation by hdeshev · Pull Request #440 · angular/zone.js

hdeshev · 2016-09-14T09:34:09Z

The nativescript-angular mobile renderer was using the zone-node.js bundle until 0.6.18+ added patching for the timers module and fetch promise-likes. Those are incompatible with NativeScript and cause runtime errors. The best solution to support zone.js in NativeScript in a way that minimizes future breakage seems to involve creating a separate bundle.

I based this bundle on the node one, just removing the unsupported patches. I also added a rudimentary test task that runs under node (to avoid spinning up device emulators and complicate build infrastructure).

nativescript-angular vendors the zone-nativescript.js bundle that this PR adds, but it would be great if we get rid of that in the future.

This PR takes care of #437 and the fetch() change is somewhat related to #439.

Avoid patching Promise twice by checking if the promise-like returned by fetch() is already an instance of ZoneAwarePromise.

googlebot · 2016-09-14T09:34:12Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2016-09-14T09:34:13Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

hdeshev · 2016-09-14T09:37:42Z

I signed it!

googlebot · 2016-09-14T09:37:44Z

CLAs look good, thanks!

googlebot · 2016-09-14T09:37:44Z

CLAs look good, thanks!

mhevery · 2016-09-15T17:41:44Z

@@ -0,0 +1,842 @@
+var Zone$1 = (function (global) {


Please Remove the changes to the dist folder. We update the dist folder on releases only.

mhevery · 2016-09-15T17:43:06Z

  jrunner.execute();
 });
+
+gulp.task('test/nativescript', ['compile'], function(cb) {


Please also update the package.json file so that native scripts can be run using npm run test-nativescript command.

mhevery · 2016-09-15T17:43:53Z

      // ignore output to prevent error;
      fetchPromise.then(() => null, () => null);
-      if (fetchPromise.constructor != NativePromise) {
+      if (fetchPromise.constructor != NativePromise && fetchPromise.constructor != ZoneAwarePromise) {


Can you break this change into a separate commit. I don't think it has anything to do with NativeScript

Already done. The PR contains both commits.

mhevery · 2016-09-15T17:44:37Z

+
+// List all tests here:
+import './common_tests';
+import './nativescript_tests';


nativescript_tests.ts seems to be an empty file. Was that intended?

I thought it would look consistent with the node_tests.ts setup, but I can delete it. Your call.

Based on the zone-node entrypoint with the timers module patching removed. Also add rudimentary support for running NativeScript tests under node.js (to avoid setting up device emulators).

fix(fetch): improve the check before patching fetch promises

6cab477

Avoid patching Promise twice by checking if the promise-like returned by fetch() is already an instance of ZoneAwarePromise.

googlebot added the cla: no label Sep 14, 2016

googlebot added cla: yes and removed cla: no labels Sep 14, 2016

mhevery suggested changes Sep 15, 2016

View reviewed changes

feat(nativescript): Add a separate zone-nativescript entrypoint

e6c01d8

Based on the zone-node entrypoint with the timers module patching removed. Also add rudimentary support for running NativeScript tests under node.js (to avoid setting up device emulators).

hdeshev force-pushed the zone-nativescript branch from 8214dea to e6c01d8 Compare September 17, 2016 12:50

mhevery approved these changes Sep 30, 2016

View reviewed changes

mhevery merged commit c4da1fa into angular:master Sep 30, 2016

manuelschneider mentioned this pull request May 3, 2017

fix double patched timers in older node versions #744

Closed

Conversation

hdeshev commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Sep 14, 2016

Uh oh!

googlebot commented Sep 14, 2016

Uh oh!

hdeshev commented Sep 14, 2016

Uh oh!

googlebot commented Sep 14, 2016

Uh oh!

googlebot commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hdeshev commented Sep 14, 2016 •

edited

Loading