i-bem__dom: update _elemCache on findElem #842

qfox · 2015-01-30T04:42:58Z

Fixes #583

coveralls · 2015-01-30T04:45:51Z

Coverage increased (+0.38%) to 83.59% when pulling e94653c on issues/583@v2 into 992df34 on v2.

coveralls · 2015-01-30T04:51:36Z

Coverage decreased (-3.09%) to 80.13% when pulling 51dc32f on issues/583@v2 into 992df34 on v2.

coveralls · 2015-01-30T06:22:19Z

Coverage decreased (-3.09%) to 80.13% when pulling 24295bf on issues/583@v2 into 992df34 on v2.

veged · 2015-01-30T08:48:14Z

common.blocks/i-bem/__dom/i-bem__dom.js

                    return _self.buildClass(name, modName, modVal);
                }).join(',.'),
            res = findDomElem(ctx, selector);

+        (ctx === this.domElem) && names.forEach(function(name) {
+                var key = _self.buildClass(name, modName, modVal),
+                    elem = this._elemCache[key] = res.filter('.' + key);


In most cases we find a single elem (names.length === 1). So we can avoid unnecessary res.filter:

var isSingleName = names.length === 1; ... elem = this._elemCache[key] = isSingleName? res : res.filter('.' + key);

Sounds reasonable, thanks!

Is there difference between name + buildModPostfix(modName, modVal) and _self.buildClass(name, modName, modVal) ?

definitely — name + buildModPostfix(modName, modVal) more like violation of encapsulation of building full class

coveralls · 2015-01-30T10:24:10Z

Coverage decreased (-3.09%) to 80.13% when pulling 0369057 on issues/583@v2 into 992df34 on v2.

narqo · 2015-03-05T15:23:18Z

/cc @dfilatov can you review it?

qfox · 2015-03-05T16:24:33Z

What's with coverage?

narqo · 2015-03-05T22:49:49Z

(just in case...): you can investigate the coverage decreasing directly on Coveralls

qfox · 2015-03-05T23:05:02Z

I didn't see anything about common.blocks/i-bem/__dom/i-bem__dom.js but see common.blocks/i-bem/i-bem.vanilla.js change.

narqo · 2015-03-06T05:44:31Z

Oh, sh.., looks like we have a fail positive here. Take a look at the build logs in Travis: there are few failed specs.

/cc @tadatuta @andrewblond We got a critical problem with enb-bem-specs here!

coveralls · 2015-03-11T13:02:00Z

Coverage increased (+0.28%) to 83.92% when pulling 46c2ec8 on issues/583@v2 into 9876bf9 on v2.

qfox · 2015-03-11T13:30:04Z

Fixed bug.
Made a little refactoring.
Squashed commits.

Need review ;-)

enb/enb-bem-specs#21

narqo · 2015-03-11T13:43:02Z

common.blocks/i-bem/__dom/i-bem__dom.js

-                names.split(' ').map(function(name) {
-                    return _self.buildClass(name, modName, modVal);
-                }).join(',.'),
+            keys = names.map(function (name) {


code style problem here

Sorry, fixed. How it doesn't caught on pre-commit?..

coveralls · 2015-03-11T14:06:16Z

Coverage increased (+0.28%) to 83.92% when pulling 0b4b89d on issues/583@v2 into 9876bf9 on v2.

blond · 2015-03-11T14:39:18Z

Oh, sh.., looks like we have a fail positive here. Take a look at the build logs in Travis: there are few failed specs.

@zxqfox, @narqo #908

narqo · 2015-03-11T21:37:42Z

common.blocks/i-bem/__dom/i-bem__dom.js

            res = findDomElem(ctx, selector);

+        // caching results if possible
+        (ctx === this.domElem) && keys.forEach(function(key, i) {


As far as I remember, it's not save to compare jQuery-nodes in this way. Check this:

var link = $('.link:eq(0)'); link === $(link); // ☚ false

Can you compare underlying DOM-nodes instead?

You're right. Need to fix that.

coveralls · 2015-03-12T02:26:13Z

Coverage decreased (-15.49%) to 68.16% when pulling 2436afc on issues/583@v2 into 9876bf9 on v2.

coveralls · 2015-03-12T02:32:12Z

Coverage increased (+0.54%) to 83.87% when pulling 0a49cc7 on issues/583@v2 into 2fe510b on v2.

dfilatov · 2015-03-12T06:25:42Z

@zxqfox I'm still puzzled how prevObject is used.

qfox · 2015-03-12T06:30:25Z

@dfilatov Idea was to remove res (placed in prevObject key) to reduce links count and make possible to collect garbage. I can drop it if you don't like it.

dfilatov · 2015-03-12T06:36:11Z

I open i-bem__dom.js with your changes and try to find substring "prevObject".
There's only occurrence:
delete elem.prevObject;

qfox · 2015-03-12T06:40:48Z

It comes from here: https://github.com/jquery/jquery/blob/2380028ec4a6a77401b867a51de26a3cb8e8d311/src/core.js#L70

dfilatov · 2015-03-12T06:41:48Z

Oh, my god. Let's not to use jquery internals.

Fixes #583

qfox · 2015-03-12T06:44:55Z

Okay, but res will never be collected ;-)

dfilatov · 2015-03-12T06:46:01Z

Why?

qfox · 2015-03-12T06:47:09Z

Because res.filter create new object for each name/key and res is stored in prevObject property in each new object. Nvm.

coveralls · 2015-03-12T06:48:37Z

Coverage increased (+0.53%) to 83.86% when pulling afd06ab on issues/583@v2 into 2fe510b on v2.

dfilatov · 2015-03-12T06:57:23Z

If I lose link to res, they will be collected, won't they?

qfox · 2015-03-12T06:59:14Z

That's why I tried to delete it. in other way it will be stored in prevObject property. Where I'm wrong? I can't get ;-)
As an opposite there is another idea to make empty chain ($()) and push there filtered elements. But is it worth?

i-bem__dom: update _elemCache on findElem

Fixes #583 Closes gh-843 Ref gh-842

Partially fixes #347 Ref bem/bem-core#842 Closes gh-634

qfox force-pushed the issues/583@v2 branch from e94653c to 51dc32f Compare January 30, 2015 04:48

qfox force-pushed the issues/583@v2 branch from 51dc32f to 24295bf Compare January 30, 2015 06:12

qfox added the v2 label Jan 30, 2015

veged reviewed Jan 30, 2015
View reviewed changes

veged added this to the 2.6 milestone Jan 30, 2015

qfox mentioned this pull request Mar 5, 2015

i-bem-dom: update _elemCache on findElem #843

Merged

narqo mentioned this pull request Mar 11, 2015

Release 2.6.0 #898

Closed

qfox force-pushed the issues/583@v2 branch from 0369057 to 46c2ec8 Compare March 11, 2015 12:56

qfox mentioned this pull request Mar 11, 2015

Failed tests doesn't affect "build" result, critical enb/enb-bem-specs#21

Closed

narqo reviewed Mar 11, 2015
View reviewed changes

qfox force-pushed the issues/583@v2 branch from 46c2ec8 to 0b4b89d Compare March 11, 2015 13:58

narqo reviewed Mar 11, 2015
View reviewed changes

qfox force-pushed the issues/583@v2 branch from 2436afc to 0a49cc7 Compare March 12, 2015 02:27

i-bem__dom: update _elemCache on findElem

afd06ab

Fixes #583

qfox force-pushed the issues/583@v2 branch from 0a49cc7 to afd06ab Compare March 12, 2015 06:43

narqo removed this from the 2.6 milestone Mar 13, 2015

narqo pushed a commit that referenced this pull request Apr 16, 2015

Merge pull request #842 from /issues/583@v2

2ce9b34

i-bem__dom: update _elemCache on findElem

narqo merged commit 2ce9b34 into v2 Apr 16, 2015

narqo deleted the issues/583@v2 branch April 16, 2015 14:13

qfox mentioned this pull request Apr 17, 2015

i-bem__dom: update _elemCache on findElem calls bem/bem-bl#634

Merged

qfox pushed a commit that referenced this pull request May 14, 2015

i-bem-dom: update _elem(s)Cache on findChildElem(s)

290aff1

Fixes #583 Closes gh-843 Ref gh-842

qfox pushed a commit to bem/bem-bl that referenced this pull request Jun 8, 2015

i-bem__dom: update _elemCache on findElem calls

8fcc49b

Partially fixes #347 Ref bem/bem-core#842 Closes gh-634

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i-bem__dom: update _elemCache on findElem #842

i-bem__dom: update _elemCache on findElem #842

qfox commented Jan 30, 2015

coveralls commented Jan 30, 2015

coveralls commented Jan 30, 2015

coveralls commented Jan 30, 2015

veged Jan 30, 2015

qfox Jan 30, 2015

qfox Jan 30, 2015

veged Jan 30, 2015

coveralls commented Jan 30, 2015

narqo commented Mar 5, 2015

qfox commented Mar 5, 2015

narqo commented Mar 5, 2015

qfox commented Mar 5, 2015

narqo commented Mar 6, 2015

coveralls commented Mar 11, 2015

qfox commented Mar 11, 2015

narqo Mar 11, 2015

qfox Mar 11, 2015

coveralls commented Mar 11, 2015

blond commented Mar 11, 2015

narqo Mar 11, 2015

qfox Mar 12, 2015

coveralls commented Mar 12, 2015

coveralls commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

coveralls commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

i-bem__dom: update _elemCache on findElem #842

i-bem__dom: update _elemCache on findElem #842

Conversation

qfox commented Jan 30, 2015

coveralls commented Jan 30, 2015

coveralls commented Jan 30, 2015

coveralls commented Jan 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 30, 2015

narqo commented Mar 5, 2015

qfox commented Mar 5, 2015

narqo commented Mar 5, 2015

qfox commented Mar 5, 2015

narqo commented Mar 6, 2015

coveralls commented Mar 11, 2015

qfox commented Mar 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 11, 2015

blond commented Mar 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 12, 2015

coveralls commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015

coveralls commented Mar 12, 2015

dfilatov commented Mar 12, 2015

qfox commented Mar 12, 2015