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

backslash in prop should work #17

Merged
merged 1 commit into from
May 18, 2016
Merged

backslash in prop should work #17

merged 1 commit into from
May 18, 2016

Conversation

stevemao
Copy link
Collaborator

Fixes #15

@SamVerschueren
Copy link
Contributor

I tried to fix this issue as well, but instead tried using a regex to split the string which results in much cleaner code.

    var pathArr = path.split(/[^\\\\]\./);

    for (var i = 0; i < pathArr.length; i++) {
        obj = obj[pathArr[i]];

        if (obj === undefined) {
            break;
        }
    }

The downside is that it isn't working entirely (yet) as it eats up the last character per token.

'foo\\.bar.baz'.split(/[^\\\\]\./);
//=> [ 'foo\\.ba', 'bar' ]

I was looking into using something like a lookbehind/lookahead, but I'm not very familiar with the concept. Just wanted to share because maybe someone else has an idea to simplify this.

@stevemao
Copy link
Collaborator Author

I tried to write a customised split (parse the string manually).
We should set up benchmark to see which solution is the best.

@hobbyquaker
Copy link

sadly js regexs don't support lookbehind. here is my for-loop-based approach to a split function. Backslash can escape dots and backslashes here.

        /**
         * Split str by dot - supports backslash escaped dots and backslashes
         * @param {string} str
         * @returns {Array.<string>}
         */
        function split(str) {
            str = '' + str;

            // use native split if possible
            if (str.indexOf('\\.') === -1) return str.split('.');

            var res = []; // the result array
            var pos = 0; // the start position of the first chunk

            function chunk(start, end) {
                // slice, unescape and push onto result array
                res.push(str.slice(start, end).replace(/\\\\/g, '\\').replace(/\\\./g, '.'));
                // set position of next chunk.
                pos = end + 1;
            }

            var esc; // boolean indicating if a dot is escaped
            var j;
            for (var i = 0, l = str.length; i < l; i++) {
                if (str[i] === '.') {
                    esc = false;
                    // walk over preceding backslashes in reverse direction
                    for (j = i - 1; str[j] === '\\'; j--) esc = !esc;
                    // dot is escaped only if preceded by an odd number of backslashes
                    if (!esc) chunk(pos, i);
                }
            }

            // process the last chunk
            chunk(pos, i);

            return res;
        }

@stevemao
Copy link
Collaborator Author

@hobbyquaker Can you do a PR and we can benchmark it. Thanks.

@hobbyquaker
Copy link

will do tomorrow :)

@hobbyquaker
Copy link

I just compared my split method, as expected it is much slower...

getPathSegments from current master branch:

         171,953 op/s » get
          75,894 op/s » set
          61,423 op/s » delete

my implementation:

          88,625 op/s » get
          44,889 op/s » set
          33,033 op/s » delete

But I think these results aren't really comparable. Current implementation only escapes dots and can't guarantee predictable results on some edge cases like

m.get({'fo.ob.az\\': {bar: true}}, 'fo\\.ob\\.az\\.bar'); // ?
m.get({'fo.ob.az.bar': true},      'fo\\.ob\\.az\\.bar');

My implementation can handle this case with escaped-backslashes:

m.get({'fo.ob.az\\': {bar: true}}, 'fo\\.ob\\.az\\\\.bar'); 
m.get({'fo.ob.az.bar': true},      'fo\\.ob\\.az\\.bar');

I would suggest before comparing performance we should clearify the escape-behaviour and implement the necessary tests. Only escaping dots (like statet in #18) isn't enough, imho backslashes themselves have to be escaped too.

@stevemao
Copy link
Collaborator Author

Can you do a PR? The current tests are not complete.

@sindresorhus
Copy link
Owner

ping @hobbyquaker :)

@stevemao
Copy link
Collaborator Author

Can we merge this and if anyone can make a faster implementation, do a follow-up PR.

@sindresorhus sindresorhus merged commit b03788e into master May 18, 2016
@sindresorhus sindresorhus deleted the backslash-in-prop branch May 18, 2016 06:04
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.

4 participants