WIP fix(lazyload onChanges) fix(WindowService) #105

MurhafSousli · 2017-02-07T19:09:46Z

Add WindowService and Universal support
Use OnChanges for lazyLoad input to update automatically when user change the image

I was wondering why did you choose ngZone to implement lazy loading outside angular! (I'm not familiar with it) but well done 👍

Use WindowService, Should work on Universal Load new image if user changed `lazyLoad` Input

I'm not sure If I supposed to put it here, but angular style guide encourges to keep tests in the same directoy

tjoskar · 2017-02-08T08:37:07Z

Are you still working on this pull request? If so, can you add WIP (work in progress) to the pull request title.

tjoskar · 2017-02-08T08:39:27Z

test/helpers/window.service.spec.ts

+/* tslint:disable:no-unused-variable */
+
+import { TestBed, async, inject } from '@angular/core/testing';
+import { WindowService } from './window.service';


Your should put the test in the /test folder instead of /test/helpers :)

tjoskar · 2017-02-08T08:41:46Z

test/helpers/window.service.spec.ts

@@ -0,0 +1,28 @@
+/* tslint:disable:no-unused-variable */


You should be able to remove this line if you start using is instead of expect. See my comment below.

Regarding the test file, I didn't write it my self, a contributor who helped in testing a module of mine wrote this, and I just add it, I'm not comfortable with testings yet

tjoskar · 2017-02-08T08:52:53Z

test/helpers/window.service.spec.ts

+
+  it('nativeWindow should be defined and equaled to "window" object', () => {
+    expect(service.nativeWindow).toBeDefined();
+    expect(service.nativeWindow).toBe(window);


See my comment above, you can not use expect here.

You can convert these two lines with:

isNot(service.nativeWindow, undefined); is(service.nativeWindow, window);

tjoskar · 2017-02-08T08:56:51Z

test/helpers/window.service.spec.ts

+  });
+
+  it('should inject the service instance...', inject([WindowService], (service: WindowService) => {
+    expect(service).toBeTruthy();


I'm using mocha in this project and have created my own assert lib, expect is therefore not defined. Take a look at scroll-listener.test.ts to see some examples.
You can however replace this line with:

isNot(service, undefined);

tjoskar

I added some comments, most of them are just quick fix so don't be scared about the number ;)

I'm using 4-spaces in this project. See my editor config. I would recommend you to use a addon to your editor. ex. for atom or for vs code
I see that you change many of my 4-space lines to 2-space. Can you please change back to 4-spaces? :)

tjoskar · 2017-02-08T09:11:11Z

src/lazyload-image.directive.ts

+import {Directive, ElementRef, Input, NgZone, SimpleChanges, OnChanges, OnDestroy} from '@angular/core';
+import {getScrollListener} from './scroll-listener';
+import {lazyLoadImage} from './lazyload-image';
+import {WindowService} from "./window.service";


You are using spaces around the imported variable in your test file but not here. Any reason?

Oh man this is the way WebStorm organizes the code! sorry for that

tjoskar · 2017-02-08T09:18:58Z

src/lazyload-image.directive.ts

+export class LazyLoadImageDirective implements OnChanges, OnDestroy {
+  @Input('lazyLoad') lazyImage;   // The image to be lazy loaded
+  @Input() errorImage: string;    // The image to be displayed if lazyImage load fails
+  @Input() scrollTarget; // Change the node we should listen for scroll events on, default is window


Please align the comment

tjoskar · 2017-02-08T09:26:39Z

src/lazyload-image.directive.ts

+    this.ngZone = ngZone;
+    // Get window object, fallback for Universal
+    this.window = window.nativeWindow || (<any>global);
+    this.scrollTarget = this.window;


This will overwrite the user input for the scrollTarget. I guess we should do something like:

this.window = window.nativeWindow || global; this.scrollTarget = this.scrollTarget || this.window;

I just noticed now, see the code

constructor{ this.window = window.nativeWindow || (<any>global); this.scrollTarget = this.window; }

scrollTarget will be window by default, but when user provide one, it will override window in OnChanges block. so I think it will work fine

It works like this, right?

the constructor is executing and assigning:

this.window = window.nativeWindow || (<any>global); this.scrollTarget = this.window;

Angular will now assign the user assignments to the properties. Angular will therefore override scrollTarget (if the user state a scrollTarget).

So yes, it will probably work just fine but it is kind of confusing. We should maybe add a comment:

this.scrollTarget = this.window; // setting default value, this will be overwritten with a user specific scroll target if any is present

tjoskar · 2017-02-08T09:28:12Z

src/lazyload-image.directive.ts

+  scrollSubscription;
+  window;
+
+  constructor(el: ElementRef, ngZone: NgZone, window: WindowService) {


Do we really need to have a WindowService? Can't we just use a function? See my comment in the service.

@tjoskar this had been talked about many times in the past, people were saying it is, but I'm not sure if things has been changed now, I use it generally to avoid issues related to AOT compiling and Universal, I also prefer "the angular way", you can ask about it in the angular room

I did it for you

Okay so there are three reasons:

Decoupling from DOM

Testability

The angular way

1. is the most important aspect but we can solve that with a ordinary function as well.
2., sure, it may be easier to test (even though we can use the DI system for function as well) but right now we do not any test where we want to mock the WindowService.
3., I agree, it is the Angular way to create a service class so I can agree to use a class in this case.

tjoskar · 2017-02-08T09:29:10Z

src/lazyload-image.ts

@@ -5,20 +5,19 @@ import 'rxjs/add/operator/mergeMap';
 import 'rxjs/add/operator/catch';
 import 'rxjs/add/observable/of';
 import { Observable } from 'rxjs/Observable';
-import { getScrollListener } from './scroll-listener';


tjoskar · 2017-02-08T09:33:37Z

src/lazyload-image.ts


-function isVisible(element: HTMLElement, threshold = 0, _window = window) {
+function isVisible(element: HTMLElement, threshold = 0, window) {


Please choose an other name for window. It is quite confusing we we are using a variable window but is is not the global window variable. Furthermore, we should add a type to the variable, like:

function isVisible(element: HTMLElement, threshold = 0, _window: Window) {

Avoid prefixing private properties and methods with an underscore.

You were using _window! I changed that according to the styleguide

Yeah, sure. _window may be a bad name. But I think window is even worse (no offence) since it shadow a global variable. Which is confusing, especially for window: http://stackoverflow.com/a/11901566/4726243

Maybe: windowRef is a better name?

tjoskar · 2017-02-08T09:38:25Z

src/lazyload-image.ts

    return (scrollObservable: Observable<Event>) => {
        return scrollObservable
-            .filter(() => isVisible(image, offset))
+            .filter(() => isVisible(image, offset, window))


As a comment above; it is confusing to have a window variable that shadowing the global variable with the same name.

you are right, it is confusing

tjoskar · 2017-02-08T09:41:40Z

src/window.service.ts

+function _window(): any {
+  // return the global native browser window object
+  return typeof window !== 'undefined' ? window : undefined;
+}


We should be able to replace the whole service with the following function, any thoughts?

export function getNativeWindow() { return typeof window !== 'undefined' ? window : undefined; }

This function is useless without a service

It is not, I could then use it in lazyload-image.directive.ts:

import { getNativeWindow } from './window.service'; ... constructor(el: ElementRef, ngZone: NgZone) { this.elementRef = el; this.ngZone = ngZone; // Get window object, fallback for Universal this.window = getNativeWindow(); this.scrollTarget = this.window; }

But as I comment above we should use a class (service) because it is the angular way.

@tjoskar The point of WindowService is to provide it in ngModule, and then inject it in the directive constructor so it can be recognized as a service. what you are doing here is getting window object by calling a static function in a static class which is the same as just using window directly

which is the same as just using window directly

Are you sure? The main problem is that node will (under the server rendering) execute some of the code and since window is not defined in the node runtime it will throw an error (ReferenceError: window is not defined), right?
If we just wrap the reference in a function like getNativeWindow above it should work? Right?

Yes you should wrap it in a function that falls back to global when window is undefined.
but my point about implementing WindowService is to make the module work the angular way. I also recommend wrapping your code into a service like LazyImageService

tjoskar · 2017-02-08T09:55:04Z

src/lazyload-image.directive.ts

        });
+      }


We need to unsubscribe to the old observable if we start a new one. We will otherwise have memory leeks and if we pass the same image element to the lazyLoadImage-function the previous image may be activated after the new images has been loaded. Meaning, we can't guarantee the loading order if we do not unsubscribe.

tjoskar · 2017-02-08T10:12:28Z

I was wondering why did you choose ngZone to implement lazy loading outside angular!

I do not use any angular feature when I load the image so I do not need to run the script inside the angular context. And beside, if I run the script inside a the Angular context, Angular will run a change detection for every scroll event for every 100 ms due to this line: https://github.com/tjoskar/ng2-lazyload-image/blob/598d39402290c9f268236c20942d8af5cb1ffed6/src/scroll-listener.ts#L11

You can also see these issues: #76 and #87

jshrssll · 2017-02-10T03:52:19Z

@MurhafSousli any update on this? Really hanging for the universal support...

MurhafSousli · 2017-02-10T21:51:38Z

@splurtcake I'm having an overload this week at work, but this PR just need those tiny changes @tjoskar mentioned, @splurtcake can you fix them?

jlcarvalho · 2017-03-16T02:08:10Z

Hi guys, I'm working on this PR. Everything is working locally, I'll write the tests asap and commit the changes.

tjoskar · 2017-03-19T10:51:58Z

@jlcarvalho, how is it going?

From my understanding it should just be to create a new function:

function getWindow(): any {
  return typeof window !== 'undefined' ? window : undefined;
}

and then using getWindow() where we are using window right now, e.g:

// lazyload-image.directive.ts

...
@Input() scrollTarget = getWindow();
...

Have you solved it in some other way?

tjoskar · 2017-05-11T18:17:19Z

This was fixed in #149 and included in version 3.1.1. Thanks.

MurhafSousli added 8 commits February 7, 2017 20:11

fix(Lazyload changed), fix(WindowService)

cd74eaa

Use WindowService, Should work on Universal Load new image if user changed `lazyLoad` Input

Imports WindowService

52c1dac

refactor(WindowService)

44bed6b

feat(WindowService)

68a93fe

WindowService test file

89b62f5

I'm not sure If I supposed to put it here, but angular style guide encourges to keep tests in the same directoy

Update lazyload-image.directive.ts

9635d7d

move window test to tests diretory

b2b2599

Create window.service.spec.ts

776705f

tjoskar reviewed Feb 8, 2017

View reviewed changes

MurhafSousli changed the title ~~fix(lazyload onChanges) fix(WindowService)~~ WIP fix(lazyload onChanges) fix(WindowService) Feb 9, 2017

tjoskar mentioned this pull request Feb 10, 2017

window is undefined in Angular 2 Universal #111

Closed

tjoskar closed this May 11, 2017


		function isVisible(element: HTMLElement, threshold = 0, _window = window) {
		function isVisible(element: HTMLElement, threshold = 0, window) {

WIP fix(lazyload onChanges) fix(WindowService) #105

WIP fix(lazyload onChanges) fix(WindowService) #105

Conversation

MurhafSousli commented Feb 7, 2017 • edited

tjoskar commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjoskar Feb 8, 2017 • edited

Choose a reason for hiding this comment

tjoskar left a comment

Choose a reason for hiding this comment

tjoskar Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjoskar Feb 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MurhafSousli Feb 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjoskar Feb 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjoskar commented Feb 8, 2017

jshrssll commented Feb 10, 2017

MurhafSousli commented Feb 10, 2017 • edited

jlcarvalho commented Mar 16, 2017

tjoskar commented Mar 19, 2017

tjoskar commented May 11, 2017 • edited

MurhafSousli commented Feb 7, 2017 •

edited

tjoskar Feb 8, 2017 •

edited

tjoskar Feb 8, 2017 •

edited

tjoskar Feb 15, 2017 •

edited

MurhafSousli Feb 9, 2017 •

edited

tjoskar Feb 15, 2017 •

edited

MurhafSousli commented Feb 10, 2017 •

edited

tjoskar commented May 11, 2017 •

edited