moribvndvs / ng2-idle

Responding to idle users in Angular (not AngularJS) applications.
https://moribvndvs.github.io/ng2-idle
Apache License 2.0
315 stars 128 forks source link

Updated for Angular 6 #94

Closed JustACodeMonkey closed 6 years ago

JustACodeMonkey commented 6 years ago

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[X ] Other... Please describe:
```Adds support for Angular 6

**What is the current behavior?** (You can also link to an open issue here)
ng2-idle does not work with Angular 6

**What is the new behavior?**
ng2-idle does work with Angular 6

**Does this PR introduce a breaking change?** (check one with "x")

[X ] Yes [ ] No



If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
This version will not work with Angular 4 and possibly Angular 5 (depending on the RxJs version)

**Other information**:
JustACodeMonkey commented 6 years ago

How do I go about resolving the yarn error? The build process is using node 6, but Angular 6 required node 8 or higher.

jturmel commented 6 years ago

Should this bump the version to 3.x.x since it will be breaking or even 6.x.x to match Angular project?

JustACodeMonkey commented 6 years ago

I would say 6 to match the convention for Angular and CLI and Material.

hudibrian commented 6 years ago

What is the likelihood of this PR getting merged in some time soon?

JustACodeMonkey commented 6 years ago

Not sure. I'll update the PR when I get to work tomorrow to the correct Angular / rxjs versions. It will still fail the yarn test above though. Still don't know how to fix that.

I'm not sure if the original creator is still maintaining this.

rsheptolut commented 6 years ago

@JustACodeMonkey thanks for sharing your effort! It has removed most of the errors from the current version of the original package, but I've used your fork directly as a package source and still getting these two errors on ng serve:

[1] ERROR in node_modules/ng-idle-src/modules/core/src/interruptsource.ts(24,9): error TS2304: Cannot find name 'Zone'.
[1] node_modules/ng-idle-src/modules/core/src/interruptsource.ts(25,7): error TS2304: Cannot find name 'Zone'.

Doesn't seem to know what Zone is... Altough I of course do have that in polyfills.ts:

import 'zone.js/dist/zone';

Any idea? Seems that this package doesn't lend itself well to adding from github directly via npm, but I don't know any other way - no idea how to package it and use in my own app. Until the author updates the npm package, that is.

JustACodeMonkey commented 6 years ago

@rsheptolut I'm not sure why you'd be getting that error. I'm currently working around the initial error by modifying ./node_modules/@ng-idle/core/src/eventtargetinteruptsource.js directly within my project (so I'm using the npm for this repository and then changing the code locally). Here is the updated file contents...

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 __());
    };
})();

import { Observable, Subscription, fromEvent, merge } from 'rxjs';
import { filter, throttleTime } from 'rxjs/operators';
import { InterruptArgs } from './interruptargs';
import { InterruptSource } from './interruptsource';
var defaultThrottleDelay = 500;
/*
 * An interrupt source on an EventTarget object, such as a Window or HTMLElement.
 */
var /*
 * An interrupt source on an EventTarget object, such as a Window or HTMLElement.
 */
EventTargetInterruptSource = (function (_super) {
    __extends(EventTargetInterruptSource, _super);
    function EventTargetInterruptSource(target, events, options) {
        var _this = _super.call(this, null, null) || this;
        _this.target = target;
        _this.events = events;
        _this.eventSubscription = new Subscription();
        if (typeof options === 'number') {
            options = { throttleDelay: options, passive: false };
        }
        options = options || { throttleDelay: defaultThrottleDelay, passive: false };
        if (options.throttleDelay === undefined || options.throttleDelay === null) {
            options.throttleDelay = defaultThrottleDelay;
        }
        _this.throttleDelay = options.throttleDelay;
        _this.passive = !!options.passive;
        var opts = _this.passive ? { passive: true } : null;
        var fromEvents = events.split(' ').map(function (eventName) { return fromEvent(target, eventName, opts); });
        _this.eventSrc = merge.apply(Observable, fromEvents);
        _this.eventSrc = _this.eventSrc.pipe(filter(function (innerArgs) { return !_this.filterEvent(innerArgs); }));
        if (_this.throttleDelay > 0) {
            _this.eventSrc = _this.eventSrc.pipe(throttleTime(_this.throttleDelay));
        }
        var handler = function (innerArgs) { return _this.onInterrupt.emit(new InterruptArgs(_this, innerArgs)); };
        _this.attachFn = function () { return _this.eventSubscription = _this.eventSrc.subscribe(handler); };
        _this.detachFn = function () { return _this.eventSubscription.unsubscribe(); };
        return _this;
    }
    /*
     * Checks to see if the event should be filtered. Always returns false unless overriden.
     * @param event - The original event object.
     * @return True if the event should be filtered (don't cause an interrupt); otherwise, false.
     */
    /*
       * Checks to see if the event should be filtered. Always returns false unless overriden.
       * @param event - The original event object.
       * @return True if the event should be filtered (don't cause an interrupt); otherwise, false.
       */
    EventTargetInterruptSource.prototype.filterEvent = /*
       * Checks to see if the event should be filtered. Always returns false unless overriden.
       * @param event - The original event object.
       * @return True if the event should be filtered (don't cause an interrupt); otherwise, false.
       */
    function (event) {
        return false;
    };
    Object.defineProperty(EventTargetInterruptSource.prototype, "options", {
        /**
         * Returns the current options being used.
         * @return {EventTargetInterruptOptions} The current option values.
         */
        get: /**
           * Returns the current options being used.
           * @return {EventTargetInterruptOptions} The current option values.
           */
        function () {
            return { throttleDelay: this.throttleDelay, passive: this.passive };
        },
        enumerable: true,
        configurable: true
    });
    return EventTargetInterruptSource;
}(InterruptSource));
/*
 * An interrupt source on an EventTarget object, such as a Window or HTMLElement.
 */
export { EventTargetInterruptSource };
//# sourceMappingURL=eventtargetinterruptsource.js.map
rsheptolut commented 6 years ago

@JustACodeMonkey thanks for the workaround, works like a charm! Hope the package author will merge the PR soon.

And I was getting an error about Zone (and that was even with the original @HackedByChinese package) because I referenced it directly from github (you can do that by running npm i JustACodeMonkey/ng2-idle --save). I get no errors when installing from npm though. Apparently, there's something in this package that's incompatible with that way of directly referencing packages from github. Maybe the fact it's kind of a package but really 2 separate modules. Or some global["Zone"]-related magic stuff. Oh well.

moribvndvs commented 6 years ago

I'll upgrade node for the repo later today.

I don't know the best way to keep up with Angular deprecations, other than to just keep rolling out breaking releases for the latest Angular. I'll have to look at the supported version matrix for Angular and put it on a roadmap.

As per questions about status of the repo: The problem is that I'm trying to get the MVP done of a huge project at work and instructing at night (burning the candle at both ends). However, I've also switched to React for most of my work. I like Angular (and this module :) ) but I'm not as plugged in anymore, which results in me being reactive.

I have a couple of ideas on this: 1) hand the project off to someone or at least take on a new lead contributor 2) I like the model this and ng-idle follows, and I think it's fairly distinct and compelling compared to other solutions. I've also replicated the pattern for a few frameworks/environments. I could create a new org called IdleJs or something like that which has consistent core modules, use cases, and implementation patterns, and then delegate framework-specific adapters for things like React and Angular. That way I can get out of the Angular community's way while keeping things consistent and opening up accessibility to other communities.

JustACodeMonkey commented 6 years ago

Thanks Mike!

CatGuardian commented 6 years ago

Can we get this merged in please?

AHelper commented 6 years ago

FYI, to update the node version used for tests, change .nvmrc to 8.11.1 to satisfy angular's node requirement of >=8.9.1 <9.0.0.

CatGuardian commented 6 years ago

@rsheptolut @JustACodeMonkey

I got the same error you were experiencing @rsheptolut and I think this can be fixed if @JustACodeMonkey modifies interruptsource.ts to have the following import at the top of the file: import 'zone.js';

But I think it might be fixed if we are able to install it correctly through npm once this is merged to master and @HackedByChinese does an npm publish.

JustACodeMonkey commented 6 years ago

@CatGuardian I made those changes and committed them to my branch. Hopefully, the addition of the zone import will solve your issue.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.003%) to 99.37% when pulling be2faf7db32bd49c69a72b150b223ce04cc88925 on JustACodeMonkey:master into c9d7ce5a7f6b242bbbc099a85e541beab8e9c8cb on HackedByChinese:master.

JustACodeMonkey commented 6 years ago

@coveralls So, how do I fix the coverage failures? They occurred in files I didn't touch.

abdulk1 commented 6 years ago

Any update on this? When is this PR getting merged?

janos-roche commented 6 years ago

@JustACodeMonkey Look at this in the coverage report. I think that's the only thing blocking you from passing. It seems you have a variable with a default value and in your test you don't cover one of the 2 possibilities, that are a) you provide the value b) you don't so it uses the default