Memory leak fix, minor bug fixs and feature suggestions

[skylinne95]

Hi,

I am using nuxt.js 2.14 with typescript and after some testing, i have noticed couple of issues with some parts of the code and i thought i might share how i fixed them for me.

Invalid import

The first issue i had is in src/polyfills.js line 1: import { polyfill } from 'seamless-scroll-polyfill/dist/esm/Element.scrollBy'.

Basically, polyfill is not exported from 'seamless-scroll-polyfill/dist/esm/Element.scrollBy' so i had to do something like:

import { polyfill } from 'seamless-scroll-polyfill or i guess you can do import { elementScrollByPolyfill } from 'seamless-scroll-polyfill.

Memory leak

The next issue is a memory leak i encountered while testing.

Basically the debounced functions were not removed in the breforeDestryoy hook because the code returns a new function rather than the reference of the ones attached in the mounted hook.

  beforeDestroy() {
    if (isClient) {
      this.observer.disconnect();
      this.$refs.vsWrapper.removeEventListener(
        'scroll',
        debounce(this.calcOnScroll, SCROLL_DEBOUNCE) // this does not remove the one attached in `mounted` hook
      );
      window.removeEventListener(
        'resize',
        debounce(this.calcOnInit, RESIZE_DEBOUNCE), // this does not remove the one attached in `mounted` hook
        false
      );
    }
  },

My solution to this issue was something like this:

  // ...
  data: () => ({
     // ...
    onScrollFn: null,
    onResizeFn: null
  }),

  mounted() {
    this.calcOnInit();

    if (isClient) {
      this.onScrollFn = debounce(this.calcOnScroll, SCROLL_DEBOUNCE);
      this.onResizeFn = debounce(this.calcOnInit, RESIZE_DEBOUNCE);

      this.attachMutationObserver();
      this.$refs.vsWrapper.addEventListener('scroll', this.onScrollFn);
      window.addEventListener('resize', this.onResizeFn, false);
    }
  },
  beforeDestroy() {
    if (isClient) {
      this.observer.disconnect();
      this.$refs.vsWrapper.removeEventListener('scroll', this.onScrollFn);
      window.removeEventListener('resize', this.onResizeFn, false);
    }
  },
  // ...

Store the function references and use the references when attaching / removing listeners.

Minor improvements

The next issues are minor but improve bound calculation precision and avoid "cannot access xyz from undefined" error

Bound calculation precision

First, i added approximatelyEqual for this.boundRight in calcBounds to improve the precision. I noticed this issue on mobile devices where the difference was +- <=1 px.

Example for the code solution:

    calcBounds() {
      this.boundLeft = this.currentPos === 0;
      this.boundRight = approximatelyEqual(
        this.wrapperScrollWidth - this.wrapperVisibleWidth,
        this.currentPos,
        5
      );

Avoiding undefined ref

I had to check wether the ref exists or not in calcOnInit and calcOnScroll. While testing on mobile devices i noticed this issue where at some point the vsWrapper was undefined while switching between portrait and landscape mode.

Code example:

    // ...
    calcOnInit() {
      if (!this.$refs.vsWrapper) {
        return;
      }
      this.calcWrapperWidth();
      this.calcSlidesWidth();
      this.calcCurrentPosition();
      this.calcBounds();
      this.calcMaxPages();
    },
    calcOnScroll() {
      if (!this.$refs.vsWrapper) {
        return;
      }
      this.calcCurrentPosition();
      this.calcBounds();
    },
    // ...

So far these were the only fixes i needed (for my use case anyway).

Hope this helps and thanks for sharing this repo :)

P.S. Oh, i almost forgot to mention some feature suggestions.

It would be awesome if you can emit events when changing the state of the bounds left and right

For my use case i needed navigation controls else were in my code so for anyone who wants to do this, those emits would help a lot since you wont have to attach more listeners to check the bounds.

[bartdominiak]

Thanks for spotting all of those issues. I really appreciate for all of your constructive feedback. I'll prepare release ASAP with all fixes as you mentioned above.

Invalid import

It looks like you're using npm, and you have installed newest seamless-scroll-polyfill@1.2.0 version, instead older 1.1.0 (There was some updates on this packages, and currently we cannot update this for now - We're working on it). I see also vue-snap package.json is pointing to ^1.1.0 (with dash) and without lock file npm will download the newest one.

Please take a look over vue-snap@0.3.4 - It should work as expected now. All other fixes will be prepared soon 👍 Thanks 🙌

[bartdominiak]

Just released vue-snap@0.4.0 with all fixes. Feature request with emits moved to #35