Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Also check element.sticky in onScrollEvents #39

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

marcveens
Copy link
Contributor

There is a possibility the sticky is destroyed while the scroll event is active. This could cause a "Cannot read property 'active' of undefined"

There is a possibility the sticky is destroyed while the scroll event is active. This could cause a "Cannot read property 'active' of undefined"
@analog-nico
Copy link

This is occurring very rarely in my production system, too. I would highly appreciate this to be merged.

A similar check would make sense for the onResizeEvents(...) function, too. But it’s probably too rare. I did not experience the same issue for this event yet. Just in case:

  onResizeEvents(element) {
+
+     if (!element.sticky) {
+         return;
+     }
+
      this.vp = this.getViewportSize();

Copy link

@analog-nico analog-nico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, element.sticky can only be falsy in case of the race condition as mentioned in the initial message for this PR. I reviewed the whole library code to verify that the component lifecycle is sound. All looks good to me!

@analog-nico
Copy link

analog-nico commented Jul 14, 2019

FYI, here is a monkey patch that I am using now:

var Sticky = require('sticky-js');

var origOnScrollEvents = Sticky.prototype.onScrollEvents;
Sticky.prototype.onScrollEvents = function (element) {
    if (element.sticky) {
        origOnScrollEvents.call(this, element);
    }
};
var origOnResizeEvents = Sticky.prototype.onResizeEvents;
Sticky.prototype.onResizeEvents = function (element) {
    if (element.sticky) {
        origOnResizeEvents.call(this, element);
    }
};

// Any new Sticky(...) calls after the above

@rgalus rgalus merged commit e1065cb into rgalus:master Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants