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

DOM attributes getting applied from an unrelated component's props, nextBase not cleaned up #769

Closed
ngokevin opened this issue Jul 24, 2017 · 3 comments
Labels

Comments

@ngokevin
Copy link

ngokevin commented Jul 24, 2017

I have attributes from a different component somehow getting applied internally to another component. Here is the code for the component; the only props it defines are id and className, but it is somehow getting position, rotation, and scale attributes from another component.

<!-- This component should not have any other props applied. -->
<Entity id={entity.id} className="entity"/>

Here is React Developer Tools showing the props. Those position, rotation, and scale props are getting applied to #entity1 somehow. It's not in the props, but it does end up in the DOM node:

screen shot 2017-07-24 at 6 31 51 am

Here's showing that those values are applied:

screen shot 2017-07-24 at 6 36 08 am

I don't believe it is any of the application code doing this. Here I am logging the attachedCallback, and these values are there from the start:

screen shot 2017-07-24 at 6 38 47 am

And then logging from Preact's afterMount, those values are there:

screen shot 2017-07-24 at 6 41 59 am

I've also confirmed that MutationObserver says those values are there when the DOM node is mounted. I've disabled my refs and checked my attributeChangedCallbacks, and they are not what's causing this. Adding key does not help.

I've tried poking around Preact's code (v8.2.1). Perhaps these values are being recycled somehow? Any tips on where to investigate?

Repo: https://github.com/ngokevin/architect
Reproduce: npm install && npm run start then head to http://localhost:7001/?recording=https://gist.githubusercontent.com/anonymous/0bb931162b5b3280ec58dba4bd0d18e2/raw/cd9115e332e5ef0373282849255e7757362987b1/clone.json. Do document.querySelector('#entity1').getAttribute('position') after the recording runs and it should return 0/0/0.

@ngokevin
Copy link
Author

ngokevin commented Jul 24, 2017

OK, working on debugging! Found that it's using an initialBase against the offending component:

screen shot 2017-07-24 at 7 44 34 am

EDIT:

I've found it was trying to reuse an existing DOM element. The diffing didn't seem to work quite right. The "base" element had attributes [id, class, geometry, material, position, rotation, scale], and my desired props were only [id, class], yet the diffing did not try to remove any of the [geometry, material, position, rotation, scale] attributes.

EDIT 2:

Adding this to idiff sort of fixes it for me. Cleans up that "base" element of leftover attributes that aren't part of the actual props. It's a band-aid, but the root problem is how we're trying to reuse a base element without cleaning it up.

function idiff(dom, vnode, context, mountAll, componentRoot) {
    let out = dom,
      prevSvgMode = isSvgMode;

+   if (dom && !dom.parentNode && vnode.attributes) {
+     let i;
+     let removeAttributes = [];
+     for (i = 0; i < dom.attributes.length; i++) {
+       if (!(dom.attributes[i].name in vnode.attributes)) {
+         removeAttributes.push(dom.attributes[i].name);
+       }
+     }
+     for (i = 0; i < removeAttributes.length; i++) {
+       dom.removeAttribute(removeAttributes[i]);
+     }
+   }
+

EDIT 3:

Also fixed doing:

function renderComponent(component, opts, mountAll, isChild) {
  if (component._disable) return;

+  component.nextBase = null;

@ngokevin ngokevin changed the title DOM attributes getting applied from an unrelated component's props DOM attributes getting applied from an unrelated component's props, nextBase not cleaned up Jul 24, 2017
@developit
Copy link
Member

@ngokevin Thanks for the debugging! It looks like this is a result of component recycling, which I'd like to remove. There is actually a branch with recycling removed - maybe it'd be worth giving it a try to see if the issue is fixed there?

@marvinhagemeister
Copy link
Member

We completely removed the component recycler for Preact X, so issues like this should not be present anymore 👍 An alpha release is already out on npm and can be installed via preact@next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants