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

[Piwik.js] Refacto selectors #8640

Closed
dhoko opened this issue Aug 25, 2015 · 6 comments
Closed

[Piwik.js] Refacto selectors #8640

dhoko opened this issue Aug 25, 2015 · 6 comments
Assignees
Labels
wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Milestone

Comments

@dhoko
Copy link

dhoko commented Aug 25, 2015

find: function (selector)
{
    // we use querySelectorAll only on document, not on nodes because of its unexpected behavior. See for
    // instance http://stackoverflow.com/questions/11503534/jquery-vs-document-queryselectorall and
    // http://jsfiddle.net/QdMc5/ and http://ejohn.org/blog/thoughts-on-queryselectorall
    if (!document.querySelectorAll || !selector) {
        return []; // we do not support all browsers
    }

    var foundNodes = document.querySelectorAll(selector);

    return this.htmlCollectionToArray(foundNodes);
},
findMultiple: function (selectors)
{
    if (!selectors || !selectors.length) {
        return [];
    }

    var index, foundNodes;
    var nodes = [];
    for (index = 0; index < selectors.length; index++) {
        foundNodes = this.find(selectors[index]);
        nodes = nodes.concat(foundNodes);
    }

    nodes = this.makeNodesUnique(nodes);

    return nodes;
},
findNodesByTagName: function (node, tagName)
{
    if (!node || !tagName || !node.getElementsByTagName) {
        return [];
    }

    var foundNodes = node.getElementsByTagName(tagName);

    return this.htmlCollectionToArray(foundNodes);
},
makeNodesUnique: function (nodes)
{
    var copy = [].concat(nodes);
    nodes.sort(function(n1, n2){
        if (n1 === n2) {
            return 0;
        }

        var index1 = indexOfArray(copy, n1);
        var index2 = indexOfArray(copy, n2);

        if (index1 === index2) {
            return 0;
        }

        return index1 > index2 ? -1 : 1;
    });

    if (nodes.length <= 1) {
        return nodes;
    }

    var index = 0;
    var numDuplicates = 0;
    var duplicates = [];
    var node;

    node = nodes[index++];

    while (node) {
        if (node === nodes[index]) {
            numDuplicates = duplicates.push(index);
        }

        node = nodes[index++] || null;
    }

    while (numDuplicates--) {
        nodes.splice(duplicates[numDuplicates], 1);
    }

    return nodes;
},
findFirstNodeHavingClass: function (node, className)
            {
                if (!node || !className) {
                    return;
                }

                if (this.hasNodeCssClass(node, className)) {
                    return node;
                }

                var nodes = this.findNodesHavingCssClass(node, className);

                if (nodes && nodes.length) {
                    return nodes[0];
                }
            },
findNodesHavingAttribute: function (nodeToSearch, attributeName, nodes)
            {
                if (!nodes) {
                    nodes = [];
                }

                if (!nodeToSearch || !attributeName) {
                    return nodes;
                }

                var children = getChildrenFromNode(nodeToSearch);

                if (!children || !children.length) {
                    return nodes;
                }

                var index, child;
                for (index = 0; index < children.length; index++) {
                    child = children[index];
                    if (this.hasNodeAttribute(child, attributeName)) {
                        nodes.push(child);
                    }

                    nodes = this.findNodesHavingAttribute(child, attributeName, nodes);
                }

                return nodes;
            }

Hi,

I think you can remove a lot of code from here by using document.querySelectorAll or document.querySelectorAll (Compat table). If you read one of the link from the find method:

document.querySelectorAll() has several inconsistencies across browsers and is not supported in older browsersThis probably won't cause any trouble anymore nowadays

So with it you won't need makeNodesUnique, and you can use findMultiple without doing a lot of queries.

PS: I think you need to support IE8 but without its support we can remove the code from findFirstNodeHavingAttribute too

@dhoko dhoko changed the title Refacto selectors [Piwik.js] Refacto selectors Aug 25, 2015
@sgiehl
Copy link
Member

sgiehl commented Aug 25, 2015

For piwik.js we need to try to support as much browsers as possible, as those browsers wouldn't be tracked correctly otherwise. So imho we should also support IE < 8.

@tsteur
Copy link
Member

tsteur commented Aug 26, 2015

With piwik.js we still support IE6 etc. Here are also some thoughts on https://github.com/piwik/piwik/blob/2.15.0-b2/misc/internal-docs/content-tracking.md#thoughts-on-piwikjs selectors and querySelectorAll(). We can't use it anytime soon, unfortunately.

@tsteur tsteur closed this as completed Aug 26, 2015
@tsteur tsteur added the wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it. label Aug 26, 2015
@tsteur tsteur added this to the 2.15.0 milestone Aug 26, 2015
@tsteur tsteur self-assigned this Aug 26, 2015
@dhoko
Copy link
Author

dhoko commented Aug 26, 2015

Yup I understand why you need to support these browsers, but you can use a modern code, then a polyfill for olders one, isn't it ?

@tsteur :

As we don't need many features we could implement it ourselves but probably needs a lot of cross-browser testing which I wanted to avoid. We'd only start with querySelectorAll() maybe. Brings also incredible performance benefits (2-10 faster than jQuery) but there might be problems see

It's faster and consistant accross browsers. It You can find some issues with it cf but if you know them it's not a problem anymore.

I think a polyfill is a good solution.

@tsteur
Copy link
Member

tsteur commented Aug 26, 2015

Yup I understand why you need to support these browsers, but you can use a modern code, then a polyfill for olders one, isn't it ?

I know, we're just going this way to keep testing etc simple. By using one code path for all it's easier for us to test etc. Eg our query code would only run on old browsers and we would most likely not notice if it is not working correctly etc. It might be possible to do if we had automated testing on older browsers but this is not the case currently

@tsteur
Copy link
Member

tsteur commented Aug 26, 2015

Is there currently a problem eg re performance etc? As mentioned, ideally it would be the case like you said but currently not really doable for us.

@dhoko
Copy link
Author

dhoko commented Aug 26, 2015

I don't think it's a problem with performance, piwik has to perform as well as it can. Dude Piwik parse the DOM (with a lot of lol 😖) ! It has to work on mobile device etc.

AMHA If you can use a modern and fast API you have to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix If you can reproduce this issue, please reopen the issue or create a new one describing it.
Projects
None yet
Development

No branches or pull requests

3 participants