tjoskar / ng-lazyload-image

🖼 A small library for lazy loading images for Angular apps with zero dependencies
https://naughty-bose-ec1cfc.netlify.com
MIT License
762 stars 142 forks source link

Image delivered by Universal are reloaded causing a flash #417

Open intellix opened 5 years ago

intellix commented 5 years ago

Universal now delivers images in the initial SSR response and it's better than before. Now I'm noticing that after JS is bootstrapped, the images are removed and re-added causing a flash and jerk of content as it replaces images with newly created ones.

Looks related to these lines where an image is created despite the image already existing and being fully created by Universal: https://github.com/tjoskar/ng-lazyload-image/blob/623fee0b5eff4711f3796468eb86d3548536abf3/src/shared-preset/preset.ts#L19-L34

I'm guessing the fix is to short-circuit by checking if element.src === imagePath and to not re-create the element. It might not be that simple since we only use img.src but the library supports much more. If you have any guidance I don't mind doing a PR.

I've updated my initial universal example: https://github.com/intellix/ng-lazyload-image-issue-414

If you add a breakpoint to the image loading code you can see the images in DOM no longer have a src attribute despite being delivered with one via universal.

Maybe related to https://github.com/tjoskar/ng-lazyload-image/issues/391

tjoskar commented 5 years ago

This is something that I have thought of but never tested out.

Let's take the different cases:

  1. <img [defaultImage]="defaultImage" [lazyLoad]="image">. This is quite easy. If element.src is set and is the same as [lazyLoad], then don't do anything, at all.

  2. <div [defaultImage]="defaultImage" [lazyLoad]="image"></div>. Quite similar to 1.. If element.style.backgroundImage === 'url(' + [lazyLoad] +')', don't do anything.

  3. <img [defaultImage]="defaultImage" [lazyLoad]="images" [useSrcset]="true">. The same again. If img.srcset === [lazyLoad], don't do anything. I guess however it will be hard to use HTTP2 Push here as long as the client doesn't send hints about the screen size.

  4. <picture>...</picture>. If the img tag in <picture> has img.srcset === [lazyLoad], then dont do anything.

So it seems to be quite easy, just a lot of different checks. One option is to check the behavior of setup, setSources and setImageAndSources but performance-wise I think it's better to just bail out if src as been set to the lazyload image.

I'm thinking of something like:

if (imageHasBeenLoaded(element, imagePath, useSrcset)) {
  return [true]; // or somthing rx js accepts in the pip function
}

at the top in lazyLoadImage and imageHasBeenLoaded can be implemented with somthing like:

function imageHasBeenLoaded(element: HTMLImageElement | HTMLDivElement, imagePath: string, useSrcset: boolean) {
  if (isImageElement(element)) {
    if (useSrcset && element.srcset === imagePath) {
      return true;
    } else if (element.src === imagePath) {
      return true;
    }
  } else if (element.style.backgroundImage === `url('${imagePath}')`) {
    return true
  }
  return false;
}

(Can we make imageHasBeenLoaded simpler and remove some branches?)

We should probably make imageHasBeenLoaded as a hook because setLoadedImage is a hook and the user could have inserted the image into the dom in different ways.

tjoskar commented 5 years ago

Regarding #391. That is a different issue. It is because we are loading the image into sepperate Image than the image that are in the dom.

intellix commented 5 years ago

Had a little try with the above but for some reason the src attribute has disappeared by the time the lazyLoad directive has booted up and there's only the lazyload attribute.

Screenshot 2019-10-24 at 10 43 33

But the src attribute is delivered via Universal as is evident from the curl/view-source:

Screenshot 2019-10-24 at 10 44 45

The original HTML before Universal delivers it looks like:

<div style="height: 50px">
  <img [offset]="100" lazyLoad="https://placehold.it/50x50">
</div>
<div style="height: 100px">
  <img [offset]="100" lazyLoad="https://placehold.it/100x100">
</div>

So I'm guessing that upon re-bootstrap the DOM is recreated and it starts lazy-loading despite having it in the initial universal response. Probably unfixable entirely without utilising something like TransferState or bypassing LazyLoad entirely when isServer.

I think the only way to fix it is via TransferState as it's the only way to tell the browser what happened previously in the Server/Universal

tjoskar commented 5 years ago

@intellix, I have not thought of TransferState. Have you used it before? (I have not). Do you think it is possible to just create two custom hooks?

(pseudo code below)

function finally(atter) {
  if (isPlatformServer(platformId)) {
    transferState.set(atter.imagePath, 'loaded');
  }
}

function setup(atter) {
  if (isPlatformBrowser(platformId)) {
    const imageState = transferState.get(atter.imagePath);
    if (imageState === 'loaded') {
      atter.element.src = atter.imagePath;
      // Do not do anything
    }
  }
}
intellix commented 5 years ago

Even something as basic as this doesn't work which leads me to think it's impossible to solve via a directive because the [src] attribute is always added dynamically on re-bootstrap:

import { Directive, Input, ElementRef } from '@angular/core';

@Directive({
  selector: '[appLazySrc]'
})
export class LazySrcDirective {

  @Input()
  set appLazySrc(value: string) {
    this.elementRef.nativeElement.setAttribute('src', value);
  }

  constructor(private elementRef: ElementRef) { }
}

I think the only way to solve it is via a Component wrapper and TransferState which I can do today to swap between an <img [src]> and <img [lazyLoad]> tag. Here's a basic version which works with a fake lazyLoad:

<img [src]="src" *ngIf="visible$ | async">
import { Component, Input, Inject, PLATFORM_ID } from '@angular/core';
import { TransferState, makeStateKey } from '@angular/platform-browser';
import { timer, of } from 'rxjs';
import { switchMap, take, mapTo } from 'rxjs/operators';
import { isPlatformServer } from '@angular/common';

const TRANSFER_STATE_KEY = makeStateKey<boolean>('lazy-img');

@Component({
  selector: 'app-lazy-img',
  templateUrl: './lazy-img.component.html',
  styleUrls: ['./lazy-img.component.scss']
})
export class LazyImgComponent {
  @Input() src: string;

  visible$ = of(isPlatformServer(this.platformId)).pipe(
    switchMap(isServer => {
      if (isServer) {
        this.transferState.set(TRANSFER_STATE_KEY, true);
      }

      if (isServer || this.transferState.hasKey(TRANSFER_STATE_KEY)) {
        return of(true);
      }

      return timer(500).pipe(mapTo(true));
    }),
    take(1),
  );

  constructor(@Inject(PLATFORM_ID) private platformId: object, private transferState: TransferState) {
  }
}