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

.data() sometimes returns incorrect values after a re-render #390

Open
zosnap opened this issue Jun 12, 2018 · 4 comments
Open

.data() sometimes returns incorrect values after a re-render #390

zosnap opened this issue Jun 12, 2018 · 4 comments
Assignees

Comments

@zosnap
Copy link

zosnap commented Jun 12, 2018

The jQuery data() method caches accessed values internally and does not update them if the underlying DOM is changed. ("The data- attributes are pulled in the first time the data property is accessed and then are no longer accessed or mutated (all data values are then stored internally in jQuery)." -- https://api.jquery.com/data/). This can result in unexpected behavior if you use attr() or setAttribute() to update the DOM and then try to get the new value using data() (e.g. https://stackoverflow.com/questions/40075669/jquery-data-function-returns-the-wrong-value). Since Torso uses setAttribute internally when re-rendering, using .data() on elements of a view can unexpectedly return incorrect info if the data attributes were changed by the re-render.

We should either fix this so that re-rendering updates the element data, or make it explicit in the documentation that it is not safe to use the data() method in situations where re-rendering changes data attributes.

See: https://github.com/vecnatechnologies/backbone-torso/blob/master/modules/templateRenderer.js#L91-L104

@beiyi0207
Copy link
Contributor

Testing examples:

Template:

<div id="container" style="padding: 50px">
  <h1 id="heading">Title</h1>
  <p id="paragraph" data-testing="set-in-template">This is a paragraph.</p>
</div>

View:

  postrender: function() {
    this.$el.find('#heading').attr('data-testing', 'set-by-attr');
    var attr = this.$el.find('#heading').attr('data-testing');
    var data = this.$el.find('#heading').data('testing');
    console.log(attr, data); // output: set-by-attr set-by-attr

    this.$el.find('#heading').attr('data-testing', 'updated-by-attr');
    var attr = this.$el.find('#heading').attr('data-testing');
    var data = this.$el.find('#heading').data('testing');
    console.log(attr, data);  // output: updated-by-attr set-by-attr

    this.$el.find('#heading').data('testing', 'set-by-data');
    var attr = this.$el.find('#heading').attr('data-testing');
    var data = this.$el.find('#heading').data('testing');
    console.log(attr, data); // output: updated-by-attr set-by-data

    this.$el.find('#paragraph').data('testing', 'set-by-data-too');
    var attr2 = this.$el.find('#paragraph').attr('data-testing');
    var data2 = this.$el.find('#paragraph').data('testing');
    console.log(attr2, data2); // output: set-in-template set-by-data-too
  }

DOM change in the browser:
In the DOM, the data attribute is only added or updated via .attr().
.data() will not add or update data attribute to the DOM

@beiyi0207
Copy link
Contributor

https://stackoverflow.com/questions/13524107/how-to-set-data-attributes-in-html-elements

jQuery itself uses the .data() method to save information under the names 'events' and 'handle', and also reserves any data name starting with an underscore ('_') for internal use.

So using .data() to set data attributes will not update the HTML element. This might make it more difficult to debug in the browser.

@beiyi0207 beiyi0207 self-assigned this Jul 6, 2018
@beiyi0207
Copy link
Contributor

Will it work if I update data on the new node to sync with the new attributes after the new attributes are set in https://github.com/vecnatechnologies/backbone-torso/blob/master/modules/templateRenderer.js#L101-L104? Something like:

    // Update data on the new node to sync with the new data attributes
    var newDataSet = newNode.dataset;
    var newDataAttr = _.keys(newDataSet);
    var data = $newNode.data();
    _.each(data, function(value, key) {
      if (_.contains(newDataAttr, key)) {
        _.each(newDataSet, function(newDataValue, newDataAttr) {
          $newNode.data(newDataAttr, newDataValue);
        });
      } else {
        $newNode.removeData(key);
      }
    });

so if we use attr() or setAttribute() to:

  • add a new data attribute, it will add the data to $newNode.data()
  • update the value of an existing data attribute, it will update the value of that data in $newNode.data()

if we use removeAttr() to:

  • remove a data attribute, it will also remove the data in $newNode.data()

Note:
this will also remove any data set directly through $newNode.data() when called, so we would want to add documentation to point this out, unless we also want to sync the template data attribute when we set something directly in data() -- but that seems to be against the design of data() which doesn't want to set data directly on an element (but in the cache instead). So I guess I'll leave it as is for now.

@zosnap
Copy link
Author

zosnap commented Sep 13, 2018

@beiyi0207 sorry it took me so long to get back around to this. I think the current behavior is acceptable, but we should probably make it more clear in the documentation that values as data- attrs in the template may not be safe to check if they change on re-render. Maybe just having this issue is enough?

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

No branches or pull requests

2 participants