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

Error when trying to add new DnDComponent while running: Uncaught TypeError: h2 is undefined #605

Closed
xdivby0 opened this issue Jan 27, 2023 · 25 comments · Fixed by #608 or #615
Closed
Labels
bug Something isn't working

Comments

@xdivby0
Copy link

xdivby0 commented Jan 27, 2023

Everything works as long as all the elements are initialized from the beginning.

As soon as I add an element on runtime via javascript (and then also successfully register in the store), the next mousedown event logs this obsfuscated error and doesn't allow any drag and drop operations (not the old elements and also not the new ones).

The logged, obfuscated error:

Uncaught TypeError: h2 is undefined
    st dflex-dnd.mjs:1
    it dflex-dnd.mjs:1
    et dflex-dnd.mjs:1
    gt dflex-dnd.mjs:1
    onMouseDown DnDComponent.tsx:83
    React 23
    hydrate entry.client.tsx:7
    startTransition React
    hydrate entry.client.tsx:6
    requestIdleCallback handler* entry.client.tsx:17
2 dflex-dnd.mjs:1:27428

I used the example code that has this nice DnDComponent for React. I also found out the line in the component that's triggering the error:

        dflexDnD = new DnD(id, { x: clientX, y: clientY }, opts); //<-- this is line 83

I also noticed that the JavaScript-added element doesn't follow the data-index count of the other elements, but starts from 0 again (each additional javascript-added element then counts up 1, 2, 3 from there on again).

If I do

        console.log(store.getContainerByID(id));
        dflexDnD = new DnD(id, { x: clientX, y: clientY }, opts);

The logged container looks normal, same as before I dynamically add an element at runtime (where everything still worked). So the store does indeed contain this newly added element. Also when I add an element via javascript and then save and reload the page, everything works again, until I add another element.

What may I have forgotten to do? Unfortunately, my whole code is way too long, but the core part is mapping a list with each element mapping to a DnDComponent. As soon as I add one item to that list via JavaScript, it seems to mess up something.

I tried to read as much as possible in the codebase about how the new DnD constructor is, but nothing jumped my eye.

@jalal246
Copy link
Member

jalal246 commented Jan 28, 2023

Adding an element dynamically after registration is not going to work because the new element won't be configured correctly in the store. So when you are adding a new one it registers as a new container (new branch) instead of adding it to its siblings. That's why it has a wrong index. You can debug it on the console $DFlex find DOMGen and then _branches to check if it's registered correctly. Or store.getSerializedElm(id)

How do you add the new element in the runtime? The ideal situation is to unregister the register again.

  React.useEffect(() => {
    if (taskRef.current) {
      store.register({ id });
    }

    return () => {
      store.unregister(id);
    };
  }, [taskRef.current]);

@xdivby0
Copy link
Author

xdivby0 commented Jan 28, 2023

Thanks Jalal!

I'm using the default code from this example. In particular, this is already here:

  React.useEffect(() => {
    if (dndCompRef.current) {
      store.register({ id, depth, readonly });
    }

    return () => {
      store.unregister(id);
    };
  }, [dndCompRef.current]);

What do need to unregister when? I tried (on adding a new element) dismounting all element's DnDComponent, waiting 500ms for good measure, so everything is cleaned up, and then remounting all components (including the dynamically added one). Same error message, I think I am missing something here.

@jalal246
Copy link
Member

jalal246 commented Jan 28, 2023

If the container has ["id-1"-"id-2"] and I am mutating a new element id-3 then I should unregister the entire branch and refresh with the new element register/unregister. I guess maybe check if you have the right key for each component if you iterate with the map and make sure the newly added element has a unique id.

@xdivby0
Copy link
Author

xdivby0 commented Jan 28, 2023

How do I unregister the entire branch? I saw that I can do store.getContainerByID(id), but not sure how to continue. Just tried unmounting the parent-container and re-adding it back in with one more item, but still no success (same issue).

And yes, the keys that I use for the map part are unique, I just displayed them next to each item for double-checking.

@jalal246
Copy link
Member

Unregister the components will automatically update the branch

 React.useEffect(() => {
    return () => {
    // Does this actually work?
    // maybe add a console message to check it.
    store.unregister(id);
    const value = store.getSerializedElm(id)
    console.log(value)    // value should be null.
    };
  }, [dndCompRef]);

@xdivby0
Copy link
Author

xdivby0 commented Jan 28, 2023

Just tried that and I didn't see something else than null logs, each item was successfully unregistered (and later re-registered)

@jalal246
Copy link
Member

I'll try to debug it locally on my end and see if it's a bug or not.

@xdivby0
Copy link
Author

xdivby0 commented Jan 28, 2023

I'll try to debug it locally on my end and see if it's a bug or not.

Very thankful for your time Jalal, I'm here on standby, let me know if I can help somehow.

@jalal246 jalal246 added the bug Something isn't working label Jan 29, 2023
@jalal246 jalal246 linked a pull request Feb 1, 2023 that will close this issue
6 tasks
@jalal246
Copy link
Member

jalal246 commented Feb 1, 2023

#608 Should fix this issue.

@xdivby0
Copy link
Author

xdivby0 commented Feb 5, 2023

#608 Should fix this issue.

Thanks Jalal, I noticed two issues:

  1. this breaks everyone's code who trusted the default of commitAfterEndingDrag to be true (which is still documented so in the docs, but doesn't seem to be the case anymore). I tried setting it explicitly, but didn't work either. Then I saw your change in DraggableInterface.ts:33 and next few lines. I tried enabling containerTransition which didn't do anything for me either (also couldn't find anything on that option in the docs).

  2. adding a new element doesn't break the existing ones but I can't move the newly created item. This is the error:

Uncaught TypeError: this.draggedElm is undefined
    ut dflex-dnd.mjs:1
    dt dflex-dnd.mjs:1
    ft dflex-dnd.mjs:1
    Et dflex-dnd.mjs:1
    onMouseDown DnDComponent.tsx:83
    callCallback2 React
    sentryWrapped helpers.ts:98
    React 14
    sentryWrapped helpers.ts:98
    _wrapEventTarget trycatch.ts:231
    instrumentDOM instrument.ts:536
    React 8
    hydrate entry.client.tsx:29
    startTransition React
    hydrate entry.client.tsx:28
    requestIdleCallback handler* entry.client.tsx:39
2 dflex-dnd.mjs:1:30127
    ut dflex-dnd.mjs:1
    dt dflex-dnd.mjs:1
    ft dflex-dnd.mjs:1
    Et dflex-dnd.mjs:1
    onMouseDown DnDComponent.tsx:83
    callCallback2 React
    sentryWrapped helpers.ts:98
    React 14
    bind_applyFunctionN self-hosted:1683
    dispatchDiscreteEvent self-hosted:1640
    sentryWrapped helpers.ts:98
    (Async: EventListener.handleEvent)
    _wrapEventTarget trycatch.ts:231
    instrumentDOM instrument.ts:536
    React 4
    forEach self-hosted:4909
    React 4
    hydrate entry.client.tsx:29
    startTransition React
    hydrate entry.client.tsx:28
    (Async: requestIdleCallback handler)
    <anonymous> entry.client.tsx:39

The first line (this.draggedElm is undefined) makes me think I forgot something. Still having the store.register and unregister inside the useState.

@jalal246
Copy link
Member

jalal246 commented Feb 5, 2023

Is this code similar to this one: https://github.com/dflex-js/dflex/blob/main/packages/dflex-dnd-playground/src/components/stream/ContinuouslyUpdatingTodo.tsx

It's important to have a unique key for react that doesn't depend on the array index.

@xdivby0
Copy link
Author

xdivby0 commented Feb 5, 2023

Yes, just tried reducing my example to a minimum:

{items.map((item, index) => (
    <DnDComponent Component="div" key={item.id} registerInput={{id: item.id}}>
      <div>
        blub
      </div>
    </DnDComponent>
))}

the DnDComponent is 99% from your example in the docs.

The item.id are generated by a collision resistant function and are unique + don't depend on the array index.

@jalal246
Copy link
Member

jalal246 commented Feb 5, 2023

If the key is the same in each iteration then react won't remove elements with identical keys. Wich means partial updating of the DOM. To enforce a total removal of elements from tree you should tell React by providing new keys so the entire children would be unmount/mount.

const items = [{id: 0}, {id: 1}, {id: 2}]
const newItems = [{id: 0}, {id: 1}, {id: 2}, {id: 3}]

Then react will keep elements 0, 1, and 2 and insert 3.

What we need is a total removal:

const items = [{id: 0}, {id: 1}, {id: 2}]
const newItems = [{id: 00}, {id: 01}, {id: 02}, {id: 03}]

Then react will insert entire new brand children.

@xdivby0
Copy link
Author

xdivby0 commented Feb 5, 2023

So to stream in a new element, we have to make all sibling elements unregister first. Got it. I got success by unmounting all elements for a short timeout so that all of them are getting remounted at once. Seems to work now.

But still, why can't I make it reconcile? I have the commit option set to enableAfterEndingDrag: true, still the order is only remained via the transform property. I even tried calling store.commit() in the onMouseUp function, still no success. Am I missing something here?

@xdivby0
Copy link
Author

xdivby0 commented Feb 7, 2023

@jalal246 do you have any idea? It's almost working, just the commit won't work

@jalal246
Copy link
Member

jalal246 commented Feb 7, 2023

I'll look into it. But if it's not working then it should throw an error. Also, it seems your process.env is not set. Setting it to development might help with error messages and debugging.

@xdivby0
Copy link
Author

xdivby0 commented Feb 8, 2023

What do you mean process.env? Do you mean NODE_ENV? As this is a frontend package you can't just expect it to be run inside process.env.NODE_ENV environment :/

I can't set process.env for an app that gets executed only inside the browser. Yes, you can hack something together in React, but not so easy in Remix for example. I'll try to create a temporary workaround just for logging, but I think you should be careful with this

@jalal246
Copy link
Member

jalal246 commented Feb 8, 2023

Running remix build compiles using the value of process.env.NODE_ENV if it corresponds with a valid mode: "production", "development" or "test". If the value of process.env.NODE_ENV is invalid, "production" is used as a default.

https://remix.run/docs/en/v1/guides/envvars

@xdivby0
Copy link
Author

xdivby0 commented Feb 8, 2023

The env variables are not in the frontend of Remix, they are only existent in the backend. Your package is run on the client where process.env is not defined (the browser doesn't know about process.env). But the argument is not about how I get it there, there probably are some hacky ways. The issue is that NODE_ENV is not something you can count on in the frontend (in general, not just for Remix).

Having said this, of course this is an open source project, and I am not paying for dflex. If you tell me this issue is not your priority at the moment and you are not going to spend time looking into why the reconciliation broke from 3.7.1 to 3.8.0 that's okay too, I'll just use up/down arrows for now then. I was just wondering because it worked fine before and I saw some commits regarding the enableAfterEndingDrag default and thought it would be easier to spot.

@jalal246
Copy link
Member

jalal246 commented Feb 8, 2023

Your app run through bundler. To help you debug your code DFlex provides a production version and a development version. The same goes for React as well. Otherwise, you won't be able to get error messages because it's a production version. I'll suggest reading the documentation posted above again. To debug and find the issue it would be helpful to produce the bug. Same for #604.

@jalal246
Copy link
Member

jalal246 commented Feb 8, 2023

Also, I am working on a better approach to deal with elements registered/unregistered but I am not sure when I am going to finish and merge. Hopefully soon.

@xdivby0
Copy link
Author

xdivby0 commented Feb 8, 2023

Your app run through bundler. To help you debug your code DFlex provides a production version and a development version. The same goes for React as well. Otherwise, you won't be able to get error messages because it's a production version. I'll suggest reading the documentation posted above again. To debug and find the issue it would be helpful to produce the bug. Same for #604.

As I said, Remix doesn't expose Envs to the frontend, especially not directly as process.env.something. See the next section of the mentioned docs above.

I don't think I can make process.env.NODE_ENV be development for your package, sorry.

Also, I am working on a better approach to deal with elements registered/unregistered but I am not sure when I am going to finish and merge. Hopefully soon.

As for this, the issue with adding the new element to the store is resolved. As you said, I had to unmount and remount every sibling. Now that works. Just the commiting doesn't work, it's just behaving as if I'd disabled commit after ending drag. Not even the store.commit() does anything (and doesn't display errors, not even minimized errors).

@jalal246
Copy link
Member

jalal246 commented Feb 8, 2023

doesn't display errors, not even minimized error.

It won't if it's a production bundle that's the point of shipping two bundles.

@xdivby0
Copy link
Author

xdivby0 commented Feb 8, 2023

doesn't display errors, not even minimized error.

It won't if it's a production bundle that's the point of shipping two bundles.

Hmm well, that's difficult then. I don't think we can solve this with remix as process.env is not available in the frontend.

@jalal246 jalal246 linked a pull request Feb 28, 2023 that will close this issue
@jalal246
Copy link
Member

@xdivby0 if this issue is not fixed by the current release feel free to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants