Full data support

[rafaelrinaldi]

Adding support to data() just like in jQuery. Wrote a new formatter, new parser and elements now have data property.
I've added tests as well, and they're all passing. Unless the ones who wasn't passing already (at least, from the version I've forked).

@mcwhittemore already made a PR on the same thing, as you can see on the issue #215 but I've used a different approach.

[davidchambers]

Would you mind rebasing these commits on top of the head of master?

[rafaelrinaldi]

Alright, done. Let me know if you need anything else.

[davidchambers]

Are you sure you pushed your branch (with --force)? GitHub is still saying that the pull request cannot be merged, and the parent of your first commit appears to be 2925cb3, which is not the current head of master.

[rafaelrinaldi]

Hum, I'm not sure what I'm missing here... I did the rebase of data-support to master and I can see the commits made to that branch over master on my fork.

[davidchambers]

Here are the commands I would run (everyone uses Git slightly differently):

git checkout master
git pull upstream master  # assuming "upstream" is your alias for MatthewMueller/cheerio
git checkout data-support
git pull --rebase . master
# resolve any merge conflicts
git push origin data-support --force  # update your remote branch (and this pull request)

[rafaelrinaldi]

Alright, just did it. Should be good to go now.

[davidchambers]

The pull request still hasn't been updated. I'll review it as is; we can easily fulfil it manually if need be.

[davidchambers]

Let's use _.each here.

[rafaelrinaldi]

I wasn't using Underscore's each because attr wasn't using it either. Thought that following the standard would be the right thing to do.

I'll add each for object loops both in data and attr.

[davidchambers]

Cool. I advocate a somewhat functional style wherever possible, and Matt hasn't yet complained. ;)

[davidchambers]

This needs to check that the key begins with "data-":

> 'foo-data-bar'.match('data-')
["data-"]

I suggest:

var match = /^data-(.*)$/.exec(key);
if (match) {
  elem.data[match[1]] = elem.attribs[key];
}

[rafaelrinaldi]

@davidchambers Just did what you asked for. Would you mind explaining me again what you want me to do regarding the pull request? I can't see anything wrong in here and I want to understand the issue.

[davidchambers]

This check needs to be anchored to the start of the string, too.

[rafaelrinaldi]

You mean it doesn't need only to check for data- but also make sure it starts with it, right?

[davidchambers]

Yes, that's right. Something like /^data-(.*)$/ should do the trick.

[fb55]

That's just awful. A regexp is way too powerful here, use key.substr(0, 5) === 'data-'.

[davidchambers]

I took it from a snippet I suggested elsewhere, to avoid doing both key.substr(0, 5) and key.substr(5):

var match = /^data-(.*)$/.exec(key);
if (match) {
  elem.data[match[1]] = elem.attribs[key];
}

[davidchambers]

Would you mind explaining me again what you want me to do regarding the pull request?

Here's the current commit graph:

master --> o  o <-- data-support
           |  |
           |  |
           o  o
           | /
           |/
           o
           |
           |
           o

Here's what I'd like to see:

              o <-- data-support
              |
              |
              o
             /
            /
master --> o
           |
           |
           o
           |
           |
           o
           |
           |
           o

[rafaelrinaldi]

So you want data-support to be merged to master?

[matthewmueller]

@rafaelrinaldi the goal is to be able to merge the pull request from the github interface. that way we can make sure there aren't any merge conflicts and tests don't break.

[davidchambers]

Sure. There will be some merge conflicts to resolve, so you might as well tackle them now rather than later.

[rafaelrinaldi]

FYI: I'm kinda busy in a project right now but I expect to finish all the stuff needed for this PR to be merged in a couple of weeks.

[pihvi]

Tried this, didn't work correctly. I have
<a href="#" data-bar=123></a>
with $('a').data('bar') jQuery gives 123 and this gives the whole a element.

[rafaelrinaldi]

@pihvi It took me a while but I did an update that fixes that. A little test snippet I just made:

var $ = require('./lib/cheerio');

var markup = '<a href="#" data-foo="bar">Link</a>';

// Ordinary operations.
console.log($(markup).text()); // Link
console.log($(markup).attr('href')); // #

// Testing data support.
console.log($(markup).data('foo')); // bar
console.log($(markup).data()); // { foo: 'bar' }

// Adding a new value.
markup = $(markup).data('lorem', 'ipsum');

console.log(markup.data()); // { foo: 'bar', lorem: 'ipsum' }

Can someone guide me on doing the proper merge/rebase for the branch I'm working on? @davidchambers maybe?

[pihvi]

Tried this and got: ReferenceError: DATA_PREFIX_REGEX is not defined
(node_modules/cheerio/lib/render.js:23:8)

[davidchambers]

@rafaelrinaldi: I suggest running the commands listed in my earlier comment.

On another note, this is a good test case to include:

> $('<div data-foo-bar="42">').data()
{fooBar: 42}

This tests both the conversion to camel case and the type coercion.

[rafaelrinaldi]

@pihvi You must use the branch data-support of my fork in order to work. DATA_PREFIX_REGEX is located at lib/utils.js.

[pihvi]

@rafaelrinaldi I pointed cheerio in npm package.json to your data-support branch and got the error. Isn't the DATA_PREFIX_REGEX variable added by you to that branch? If that's so, the error can not occur unless your branch would be in use.

[lbeschastny]

@rafaelrinaldi here is the troubling line:
https://github.com/rafaelrinaldi/cheerio/blob/data-support/lib/render.js#L23

Looks like you forgot to require it in render.js:

var DATA_PREFIX_REGEX = require('./utils').DATA_PREFIX_REGEX;

[andineck]

Since I need this functionality as well and couldn't wait on a solution on this, I polished @rafaelrinaldi solution so that it can be merged.

[buschtoens]

Close?