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

Reinstate the "prevent unwanted clearInterval" PR #3225

Closed
cartant opened this issue Jan 13, 2018 · 3 comments
Closed

Reinstate the "prevent unwanted clearInterval" PR #3225

cartant opened this issue Jan 13, 2018 · 3 comments

Comments

@cartant
Copy link
Collaborator

cartant commented Jan 13, 2018

I left a comment on the PR that reverted PR #3044. After Zone.js 0.8.19 was released I looked into the reported Angular problem and discovered that PR #3044 was not the cause.

The comment is repeated below.


FYI, the problem with 5.5.3 is unrelated to PR #3044 and it was not the reversion of that PR that solved the problem of Angular's routing module effecting an EmptyError.

Instead, the problem appears to have been the way in which the .js files in 5.5.3 were generated.

I can reproduce the effected error by taking the util/EmptyError.js file from 5.5.3 and overwriting the file in the 5.5.2 distribution.

In fact, if you look at the files that are incorporated into the Plunk, you will see that scheduler/AsyncAction.js - the file modified in PR #3044 - does not even form part of the build.

The 5.5.2 EmptyError.js file looks like this:

"use strict";
var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
/**
 * An error thrown when an Observable or a sequence was queried but has no
 * elements.
 *
 * @see {@link first}
 * @see {@link last}
 * @see {@link single}
 *
 * @class EmptyError
 */
var EmptyError = (function (_super) {
    __extends(EmptyError, _super);
    function EmptyError() {
        var err = _super.call(this, 'no elements in sequence');
        this.name = err.name = 'EmptyError';
        this.stack = err.stack;
        this.message = err.message;
    }
    return EmptyError;
}(Error));
exports.EmptyError = EmptyError;
//# sourceMappingURL=EmptyError.js.map

Whereas the 5.5.3 EmptyError.js file looks like this:

"use strict";
var __extends = (this && this.__extends) || (function () {
    var extendStatics = Object.setPrototypeOf ||
        ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
        function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
    return function (d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();
Object.defineProperty(exports, "__esModule", { value: true });
/**
 * An error thrown when an Observable or a sequence was queried but has no
 * elements.
 *
 * @see {@link first}
 * @see {@link last}
 * @see {@link single}
 *
 * @class EmptyError
 */
var EmptyError = /** @class */ (function (_super) {
    __extends(EmptyError, _super);
    function EmptyError() {
        var _this = this;
        var err = _this = _super.call(this, 'no elements in sequence') || this;
        _this.name = err.name = 'EmptyError';
        _this.stack = err.stack;
        _this.message = err.message;
        return _this;
    }
    return EmptyError;
}(Error));
exports.EmptyError = EmptyError;
//# sourceMappingURL=EmptyError.js.map

The EmptyError implementation in 5.5.3 seems to mess with this instanceof test in Angular's router.

Why the two generated files differ, I have no idea. The EmptyError.js generated for 5.5.4 is identical to that generated for 5.5.2, yet there appear to be no configuration changes between 5.5.3 and 5.5.4, so I can only presume that the build environment for 5.5.3 was not what it should have been.

The Zone.js bug - in the PR that I referenced above - does not effect the EmptyError. The bug is related to my PR, as there was a (far more subtle) problem with Zone.js and setInterval, but that has been fixed with the release of version 0.8.19 of Zone.js.

Given that the reported problem was unrelated to the reverted PR and that the PR fixes a problem with intervals drifting (see this issue and this SO question) I think the commit should be reinstated. If not into stable, then definitely into master.

@benlesh
Copy link
Member

benlesh commented Jan 16, 2018

That... might have been the time I built 5.5 with the wrong version of TypeScript. 😬

That said, we need to be careful with 6.0, as it's using a newer version of TypeScript.

@cartant cartant closed this as completed Jan 16, 2018
@cartant
Copy link
Collaborator Author

cartant commented Jan 17, 2018

@benlesh FYI, the internal/util/EmptyError.js generated by the version of TypeScript used with 6.0.0-alpha.2 is fine.

It's different to the 5.5.2 EmptyError.js and is more similar to the 5.5.3 EmptyError.js, but it includes a call to Object.setPrototypeOf(_this, EmptyError.prototype); which suggests it will work fine with instanceof EmptyError.

I guess there was a TypeScript bug in an earlier version that has since been fixed.

Actually, the Object.setPrototypeOf is in EmptyError.ts. Hmm. A workaround for a TypeScript bug?

I guess it's this: Extending built-ins like Error, Array, and Map may no longer work. The changes relating to which were made in 2f395da.

Anyway, 6.0 will be fine.

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants