Remove nodes from their previous structures #350

jugglinmike · 2014-01-02T23:34:44Z

When manipulation methods operate on existing nodes, ensure that these
nodes are removed from their previous structures.

This should resolve issue #166.

kpdecker · 2014-02-22T21:59:04Z

lib/api/manipulation.js

+    }
+  }
+
+  return this.splice.apply(this, spliceArgs);


This return value is never used is it? Seems like overhead.

doh splice vs. slice. The call is needed but it seems like the return itself is not.

What happens if spliceIdx > instances of prevIdx that are removed? Is accounting needed there?

kpdecker · 2014-02-22T22:04:31Z

@matthewmueller seeing this in the wild with https://github.com/walmartlabs/fruit-loops what are the chances of getting this in?

fb55 · 2014-02-23T00:01:44Z

I'm really wondering why this wasn't at least reviewed, as well as several of @jugglinmike's other pull requests. It's true that there is no reason to .call() uniqueSplice, but once that's fixed it LGTM.

jugglinmike · 2014-02-23T19:36:35Z

This seems like a good idea, and I'd be happy to incorporate the feedback. As it stands, the patch conflicts with master, so I need to rebase anyway.

Unfortunately, I'm on a new system that is reporting test failures on master. TravisCI verifies the build as of yesterday, so I expect there's something up with my environment. It seems to be creating array-likes in place of arrays--here's an example error:

  7) $(...) .replaceWith (fn) : should invoke the callback with the correct argument and context:

      Error: expected [ { '0': 0 }, { '0': 1 }, { '0': 2 } ] to sort of equal [ [ 0 ], [ 1 ], [ 2 ] ]
      + expected - actual

      +"[\n  [\n    0\n  ],\n  [\n    1\n  ],\n  [\n    2\n  ]\n]"
      -"[\n  {\n    \"0\": 0\n  },\n  {\n    \"0\": 1\n  },\n  {\n    \"0\": 2\n  }\n]"

      at Assertion.assert (/home/mike/projects/cheerio/node_modules/expect.js/index.js:96:13)
      at Assertion.eql (/home/mike/projects/cheerio/node_modules/expect.js/index.js:230:10)
      at Context.<anonymous> (/home/mike/projects/cheerio/test/api.manipulation.js:545:23)
      at Test.Runnable.run (/home/mike/projects/cheerio/node_modules/mocha/lib/runnable.js:221:32)
      at Runner.runTest (/home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:374:10)
      at /home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:452:12
      at next (/home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:299:14)
      at /home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:309:7
      at next (/home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:247:23)
      at Object._onImmediate (/home/mike/projects/cheerio/node_modules/mocha/lib/runner.js:276:5)
      at processImmediate [as _immediateCallback] (timers.js:330:15)

Strange. Any thoughts?

$ uname -a
Linux tiny-pants 3.2.0-59-generic #90-Ubuntu SMP Tue Jan 7 22:43:51 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
$ node --version
v0.10.25
$ npm ls
cheerio@0.13.1 /home/mike/projects/cheerio
├── benchmark@1.0.0
├─┬ CSSselect@0.4.1
│ ├── CSSwhat@0.4.2
│ └─┬ domutils@1.4.0
│   └── domelementtype@1.1.1
├── entities@0.3.0
├── expect.js@0.3.1
├─┬ htmlparser2@3.4.0
│ ├── domelementtype@1.1.1
│ ├── domhandler@2.2.0
│ ├── domutils@1.3.0
│ └─┬ readable-stream@1.1.11
│   ├── core-util-is@1.0.1
│   ├── debuglog@0.0.2
│   └── string_decoder@0.10.25-1
├─┬ jsdom@0.8.11
│ ├─┬ contextify@0.1.6
│ │ └── bindings@1.1.1
│ ├── cssom@0.3.0
│ ├── cssstyle@0.2.9
│ ├── nwmatcher@1.3.1
│ ├─┬ request@2.34.0
│ │ ├── aws-sign2@0.5.0
│ │ ├── forever-agent@0.5.2
│ │ ├─┬ form-data@0.1.2
│ │ │ ├── async@0.2.10
│ │ │ └─┬ combined-stream@0.0.4
│ │ │   └── delayed-stream@0.0.5
│ │ ├─┬ hawk@1.0.0
│ │ │ ├── boom@0.4.2
│ │ │ ├── cryptiles@0.2.2
│ │ │ ├── hoek@0.9.1
│ │ │ └── sntp@0.2.4
│ │ ├─┬ http-signature@0.10.0
│ │ │ ├── asn1@0.1.11
│ │ │ ├── assert-plus@0.1.2
│ │ │ └── ctype@0.5.2
│ │ ├── json-stringify-safe@5.0.0
│ │ ├── mime@1.2.11
│ │ ├── node-uuid@1.4.1
│ │ ├── oauth-sign@0.3.0
│ │ ├── qs@0.6.6
│ │ ├─┬ tough-cookie@0.12.1
│ │ │ └── punycode@1.2.4
│ │ └── tunnel-agent@0.3.0
│ └── xmlhttprequest@1.6.0
├─┬ jshint@2.3.0
│ ├─┬ cli@0.4.5
│ │ └─┬ glob@3.2.8
│ │   └── inherits@2.0.1
│ ├── console-browserify@0.1.6
│ ├─┬ minimatch@0.2.14
│ │ ├── lru-cache@2.5.0
│ │ └── sigmund@1.0.0
│ ├── shelljs@0.1.4
│ └── underscore@1.4.4
├─┬ mocha@1.17.1
│ ├── commander@2.0.0
│ ├── debug@0.7.4
│ ├── diff@1.0.7
│ ├─┬ glob@3.2.3
│ │ ├── graceful-fs@2.0.2
│ │ ├── inherits@2.0.1
│ │ └─┬ minimatch@0.2.14
│ │   ├── lru-cache@2.5.0
│ │   └── sigmund@1.0.0
│ ├── growl@1.7.0
│ ├─┬ jade@0.26.3
│ │ ├── commander@0.6.1
│ │ └── mkdirp@0.3.0
│ └── mkdirp@0.3.5
└── underscore@1.5.2

fb55 · 2014-02-23T20:15:26Z

Same error here. Were any of the dependencies updated recently?

jugglinmike · 2014-02-23T20:26:44Z

Searching now...

fb55 · 2014-02-23T20:49:03Z

Any process? I got bored and started closing issues :P

jugglinmike · 2014-02-23T20:52:12Z

I saw that--thanks! No news yet, though... htmlparser2's tests are passing, so I'm not sure where this could be coming from

fb55 · 2014-02-23T20:54:31Z

My guess was that eg. expect.js changed. My modules weren't updated in a way that explains this for two months at least.

jugglinmike · 2014-02-23T20:55:40Z

I wasn't expecting your modules so much as the ones you depend on, but that's a good idea--I'll look at expect.

Although, I just triggered another build on TravisCI, and we're still passing there.

jugglinmike · 2014-02-23T20:57:15Z

$ npm show expect
npm http GET https://registry.npmjs.org/expect
npm http 200 https://registry.npmjs.org/expect

{ name: 'expect',
  description: 'the essential JavaScript test library',
  'dist-tags': { latest: '0.0.2' },
  versions: [ '0.0.0', '0.0.2' ],
  maintainers: 'onirame <enrico.marino@email.com>',
  time: 
   { modified: '2013-07-24T21:17:14.570Z',
     created: '2011-11-29T18:50:39.841Z',
     '0.0.0': '2011-11-29T18:50:42.029Z',
     '0.0.2': '2011-12-03T18:40:52.830Z' },
  author: 'Enrico Marino <enrico.marino@email.com> (onirame.no.de)',
  repository: 
   { type: 'git',
     url: 'git://github.com/onirame/expect.git' },
  users: {},
  version: '0.0.2',
  homepage: 'https://github.com/onirame/expect',
  main: 'expect.js',
  engines: { node: '*' },
  dependencies: {},
  devDependencies: {},
  dist: 
   { shasum: '4701a0f48e92c02f00d55ea29bf7d43c750bae22',
     tarball: 'http://registry.npmjs.org/expect/-/expect-0.0.2.tgz' },
  directories: {} }

Seems stable

fb55 · 2014-02-23T20:58:45Z

cheerio is using expect__.js__ :)

Hm. Travis uses node 0.10.25, I have 0.10.26 installed. Maybe node broke?

fb55 · 2014-02-23T20:59:58Z

Actually, Travis failed, too. Have a look at the logs.

jugglinmike · 2014-02-23T21:00:38Z

I started thinking this, too. But as reported above, I'm also on 0.10.25

jugglinmike · 2014-02-23T21:01:17Z

Actually, Travis failed, too. Have a look at the logs.

Oh, wow. So the test run is failing, but Mocha is exiting with status code 0...

jugglinmike · 2014-02-23T21:03:55Z

It's totally expect.js

jugglinmike · 2014-02-23T21:15:27Z

This may have been introduced with that commit, but something is may be wrong with expect.js. If you run npm install expect.js@0.2.0, then master passes. There's nothing in expect.js's release notes about a breaking change like this, so I think it's a bug with that lib.

fb55 · 2014-02-23T21:19:08Z

Well, eql was apparently changed, which seems to have caused the problems. Do you create an issue?

edit: This is what caused the bug: Automattic/expect.js@7c20523

jugglinmike · 2014-02-23T21:22:59Z

Not yet--I'm making a patch instead

jugglinmike · 2014-02-23T21:34:45Z

Actually, if this is a bug, it's with Node.js's assert module. expect.js maintains a copy of the deepEqual method, which is not reflective. I'm going to follow up on the Node.js project itself, but even if this is recognized as a bug, the "fix" will entail never considering an arguments object to be "deeply equal to" an array.

For our part, we need to slice any arguments objects before it is safe to compare to an array.

fb55 · 2014-02-25T21:44:11Z

While your pull request is being processed, I pinned the expect.js version to 0.2 (f11de45) - we should be save now.

But we actually got a second problem: Why do the tests fail silently?

jugglinmike · 2014-02-26T15:32:16Z

@fb55 Mocha is silently failing when it tries to report string diffs for objects with circular references. I've submitted a patch here

When manipulation methods operate on existing nodes, ensure that these nodes are removed from their previous structures.

jugglinmike · 2014-02-28T02:45:21Z

Alright, I've swapped out the call-invocation for direct function invocation. I've also expanded the in-line docs to cover the arguments. @fb55 mind giving it one more look and merging?

Remove nodes from their previous structures

fb55 · 2014-02-28T08:38:29Z

Building fails on node@0.8 ("unsigned certificate in chain").

kpdecker reviewed Feb 22, 2014
View reviewed changes

jugglinmike mentioned this pull request Feb 23, 2014

Enforce arguments constraint reflexively Automattic/expect.js#102

Open

Remove nodes from their previous structures

66c4a47

When manipulation methods operate on existing nodes, ensure that these nodes are removed from their previous structures.

jugglinmike mentioned this pull request Feb 28, 2014

Update to latest version of expect.js #400

Merged

fb55 added a commit that referenced this pull request Feb 28, 2014

Merge pull request #350 from jugglinmike/cleanup-manip

18bc828

Remove nodes from their previous structures

fb55 merged commit 18bc828 into cheeriojs:master Feb 28, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nodes from their previous structures #350

Remove nodes from their previous structures #350

jugglinmike commented Jan 2, 2014

kpdecker Feb 22, 2014

kpdecker Feb 22, 2014

kpdecker Feb 22, 2014

kpdecker commented Feb 22, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 25, 2014

jugglinmike commented Feb 26, 2014

jugglinmike commented Feb 28, 2014

fb55 commented Feb 28, 2014

Remove nodes from their previous structures #350

Remove nodes from their previous structures #350

Conversation

jugglinmike commented Jan 2, 2014

kpdecker Feb 22, 2014

Choose a reason for hiding this comment

kpdecker Feb 22, 2014

Choose a reason for hiding this comment

kpdecker Feb 22, 2014

Choose a reason for hiding this comment

kpdecker commented Feb 22, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

jugglinmike commented Feb 23, 2014

fb55 commented Feb 25, 2014

jugglinmike commented Feb 26, 2014

jugglinmike commented Feb 28, 2014

fb55 commented Feb 28, 2014