Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removeObserver() not working #69

Closed
webxl opened this issue Apr 4, 2017 · 6 comments
Closed

removeObserver() not working #69

webxl opened this issue Apr 4, 2017 · 6 comments

Comments

@webxl
Copy link
Contributor

webxl commented Apr 4, 2017

I was trying to disable the global mutation observer, but it's not working. globalObserver is never assigned to the static observer property:

static initHtmlApi() {
    // MutationObserver is IE11+
    if (typeof MutationObserver !== 'undefined') {
      // Mutation observer to observe dynamically added elements
      const globalObserver = new MutationObserver(mutations => {

...

static removeObserver() {
    this.observer && this.observer.disconnect();
  }

Also, there should be a way to have opt-in global observers in addition to opt-out. I know there's the convenience factor, but for pages with a lot of DOM manipulation, this feature is very costly. so that would require manual instantiation. Perhaps there could be an alternate module, and you would import it with something like import NotSoSimpleBar from 'simplebar/manual'; and they could share the bulk of code in some other module.

@webxl webxl changed the title removeObserver not working removeObserver() not working Apr 4, 2017
@Grsmto
Copy link
Owner

Grsmto commented Apr 4, 2017

Good point. I thought about that but as a first version I didn't implement it until I actually figure out some cases where we have performances issues with it.
Did you encounter a case where it actually impacts on performances?
As the observer is naturally debounced it shouldn't have a huge impact even if the page has lot of DOM manipulations but I didn't have such a scenario yet.

@webxl
Copy link
Contributor Author

webxl commented Apr 4, 2017

Yes, I have a table built with marionette.js and it has infinite scroll to show additional search results. So anywhere from 100 to a couple thousand mutations on a single query. Every time the user modifies the query, the current results get removed and another 100 get added. Within the table rows, there are a bunch of elements, too. So the events start to pile up.

@Grsmto
Copy link
Owner

Grsmto commented Apr 16, 2017

Hi, I just release a new version 2.3.0 that should solve this issue.
I leave this issue open as I still want to work on performances and to have a separate module without the mutationObservers.

@webxl
Copy link
Contributor Author

webxl commented Apr 17, 2017

Thanks. I spent some time on splitting the declarative (watching for the data-simplebar attribute) feature out last week: https://github.com/webxl/simplebar/tree/split-out-declarative. Not sure if you want to do it that way, but it might give you some ideas. I also worked on adding mocha tests, but I'm having trouble testing the mutation observer: https://github.com/webxl/simplebar/tree/unit-tests. I like testing things in node.js if possible, but for this, I might need to switch to karma-webpack so that we get the full browser API.

@Grsmto
Copy link
Owner

Grsmto commented Apr 17, 2017

Very interesting stuff. I liked the approach you proposed in your first comment with a "sub-module" simplebar/manual. This should be doable without having to duplicate some code I guess.
About the tests that's awesome! For the mutation observer I guess that's really tricky to test as you would have to mock it or something...

@webxl
Copy link
Contributor Author

webxl commented Apr 17, 2017

Yeah, the naming is a bit odd. I wanted to keep it backward compatible. It's still loaded via a submodule though: https://github.com/webxl/simplebar/blob/unit-tests/test/imperative.js#L2

@Grsmto Grsmto closed this as completed Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants