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

Allow the observed node to be specified and the selector scan search pattern to be configured. #19

Merged
merged 4 commits into from
Feb 21, 2018

Conversation

bezborodow
Copy link
Collaborator

Support scanning of only addedNodes in the mutation instead of scanning the entire document for matching selectors.

Support scanning of only addedNodes in the mutation instead of
scanning the entire document for matching selectors.
@bezborodow
Copy link
Collaborator Author

This is for #17

@bezborodow bezborodow mentioned this pull request Feb 20, 2018
@pie6k
Copy link
Owner

pie6k commented Feb 20, 2018

Maybe it'd be better idea to add option like rootNode (that obviously can be document)?

scanDocument could be confusing and seems to solve only part of the problem with partial watching for mutations.

@bezborodow
Copy link
Collaborator Author

bezborodow commented Feb 20, 2018

Edit: Warning: This is superseded by #21.

Hi @pie6k, yes, we could probably improve performance by passing in a root node to observe. However, I think this solution is valid also. They solve two different separate performance issues: 1) What object to observe for mutations, and 2) Where to scan for selectors when a mutation is observed. This change solves the latter by making it possible to only scan the added nodes in the mutation (instead of the entire document.)

This pull request allows, for example:

  1. rootNode is observed (usually it would be document.documentElement)
  2. There is a <table> in the body.
  3. An initializer with a selector tr.exampleClass is registered.
  4. A row <tr class="exampleClass"> is added to the table.
  5. At this point, a mutation event is triggered, which will have:
    • target: document.documentElement
    • addedNodes: [tr]
  6. Find nodes matching selector:
    • If scanDocument is true, search the entire document:
      • $(selector)
    • If scanDocument is false, search added nodes:
      • $(addedNodes[n]).find(selector).addBack(selector)

With this change, if scanDocument is false, then the mutation observer will only search for matching selectors within addedNodes.

If we wanted to implement your suggestion alongside this, we could improve this even further. For example:

  1. There is a <table> in the body.
  2. An initializer with a selector tr.exampleClass is registered, observing the <table> element instead of the entire document. (rootNode = table)
  3. A row <tr class="exampleClass"> is added to the table.
  4. At this point, a mutation event is triggered, which will have:
    • target: table note that target will be the same as rootNode
    • addedNodes: [tr]
  5. Find nodes matching selector:
    • If scanDocument is true, search the only the observed element:
      • $(target).find(selector)
    • If scanDocument is false, search added nodes:
      • $(addedNodes[n]).find(selector).addBack(selector)

Differences are in bold.

So, there is potential to improve two things: 1) which element mutations are observed on, and 2) how to match selectors when the mutation event is captured.

I will see if I can improve this pull request further to make both features possible.

// For each mutation.
for (var m = 0; m < mutations.length; m++) {
// If mutation type is observed.
if ($.inArray(mutations[m].type, mtypes) != -1) {
Copy link
Owner

@pie6k pie6k Feb 20, 2018

Choose a reason for hiding this comment

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

For such code I'd consider negative if's eg

if (!$.inArray(mutations[m].type, mtypes) != -1) {
  continue;
}
// rest of the code. HEY! we caved some tabs in indent!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update accordingly as I implement your other concerns.

Copy link
Owner

Choose a reason for hiding this comment

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

Don't forget to add yourself to contributors as well as some note in the README

@bezborodow
Copy link
Collaborator Author

I might also need to rename the scanDocument option to something more meaningful.

@bezborodow
Copy link
Collaborator Author

So, rootNode would be called target. I'll use that terminology from now on.

MutationObserver.observe(Node target, MutationObserverInit options)

@bezborodow
Copy link
Collaborator Author

Oops, not entirely correct. If the mutation is an attributes mutation, then target would be the node that the attribute was changed on.

@bezborodow
Copy link
Collaborator Author

I have updated the pull request to allow the "root node" to be specified and will now test it in my company's application, to see how it affects performance. Then, I might consider renaming the options after that. See also, test2.html.

@bezborodow bezborodow changed the title Implement options.scanDocument. Allow the observed node to be specified and allow the selector scan search pattern to be configured. Feb 20, 2018
@bezborodow bezborodow changed the title Allow the observed node to be specified and allow the selector scan search pattern to be configured. Allow the observed node to be specified and the selector scan search pattern to be configured. Feb 20, 2018
@bezborodow
Copy link
Collaborator Author

bezborodow commented Feb 20, 2018

Edit: Warning: This is superseded by #21.

@pie6k I have this working now and is ready for testing.

I had to use JavaScript that may break older browsers (that is, Array.forEach(), Element.querySelectorAll() and Element.matches())

There options are now:

options.target: Document | Element                        The element to observe mutations on.
                                                          Defaults to document.documentElement

options.scanMode: 'target' | 'descendants' | 'exact'      Selector search strategy.

Explanation of the different scan modes are:

  • target: Query the target element (by default, the entire document) for elements that match the given selector. (This is current behaviour.)
  • descendants: Query the node that was mutated or added to the object model for elements that match the given selector (including the mutated node and child nodes.)
  • exact: Check if the node that was mutated or added to the object model matches the selector. This does not include child nodes.

This should provide a large degree of control to the developer. Unfortunately, the best way to understand these scanModes is by using them. See the example in test2.html, and try using different scanModes.

@bezborodow
Copy link
Collaborator Author

After you are happy with this, I can document it in the README before merging.

@pie6k
Copy link
Owner

pie6k commented Feb 21, 2018

Seems good. I think it'd be good to provide links to proper demos in README

@bezborodow
Copy link
Collaborator Author

Did you want to host the demo somewhere? GitHub Pages maybe?

@pie6k
Copy link
Owner

pie6k commented Feb 21, 2018

Yup, I think GH is totally fine. Thanks for your help @dbezborodovrp

@bezborodow
Copy link
Collaborator Author

I opened a separate issue for documentation, #20.

@pie6k pie6k merged commit 764f068 into pie6k:master Feb 21, 2018
@pie6k
Copy link
Owner

pie6k commented Feb 21, 2018

Cool, merged 🎉

@bezborodow
Copy link
Collaborator Author

Cool. I think I could have actually simplified this. I might open a second PR if you can hold off tagging a new version number.

@bezborodow
Copy link
Collaborator Author

See #21 for simplification of this feature.

@bezborodow bezborodow mentioned this pull request Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants