Events double fired in Root and @keyframes render to string with <Style> as [object Object]

[kurdin]

Thanks for nice lib and I found 2 issues with react-shade:

1. All events in Root is double fired

in simple app like this, I see TEST Clicked logged twice in console

render(
  <Root>
    <button
      onClick={() => {
        console.log('TEST Clicked');
      }}
    >
      TEST
    </button>
  </Root>,
  $root
);

to fix this, I had to remove retarget method in Root.ts line 40 // retarget(shadowRoot);
and fix line 57 {state.shadowRoot && createPortal(props.children, state.shadowRoot)}

2. If I use styles object with @keyframe or @media props in <Style> component, their content renders in style's tag as [object Object] because it has deeper nested object then regular style's rule. I found work around by moving all @keyframe or @media directly to <styles> but I think you need to fix it somewhere in internal/css.ts by checking if value is Object then do another Object.keys if it is.

[jasco]

@kurdin Thank you for posting this. I have a case where onClick events are being dispatched twice. Removing the retarget call seems to resolve the problem. In limited testing I have not noticed any other side effects.

Could you clarify what you changed about the createPortal call? The line numbers you mention do not seem to align precisely with the source from the time of your post, as near as I can tell.

I am also trying to make sure I understand why the retarget call might have been needed before but is no longer needed now so I have more confidence that there will not be any side-effects. Do you happen to know what might have changed?

(For reference I am using this library with react and react-dom version 16.12.0.)

p.s. I wonder if the change that eliminated the need for retarget might have been facebook/react#11927 which was applied to react-dom v16.5.0.

edit: clarified patched revision.

[kurdin]

@jasco I am using latest react-dom as well. I did not notice any side effects of removing retarget.

for createPortal change, I don't remember exactly why, maybe just to make things simpler I changed

{state.shadowRoot
            ? createPortal(props.children, state.shadowRoot)
            : props.children}

to

{state.shadowRoot && createPortal(props.children, state.shadowRoot)}

I am always use shadowRoot for my app and I don't need to render children if shadowRoot does not exist, but again, I think this is not necessary.

Do you happen to know what might have changed?

I have no idea, sorry, it works with latest React and that all I need.

[treshugart]

Thanks for reporting this issue. It's been awhile since I've had to dive deep into this codebase, so I'm still getting warmed up.

for createPortal change, I don't remember exactly why, maybe just to make things simpler I changed

Changing it to remove the rendering of props.children might break SSR as it would need to render children, perform a state change and only then create a shadow root.

@kurdin could you please:

• Confirm whether or not this change is necessary?
• Confirm whether or not this is happening for other events besides onClick?

[kurdin]

@treshugart

Changing it to remove the rendering of props.children might break SSR as it would need to render children, perform a state change and only then create a shadow root.

Yeah, you 100% right. I did not think about SSR, I don't need it for my app.

I could not confirm that removing retarget will fix all events double fire but it fixed onClick problem for me. I am not that deep to understanding why retarget was necessary in first place. May be as @jasco mentions you need this for react-dom v16.5.0 and it was fixed in v16.12.0

p.s. I wonder if the change that eliminated the need for retarget might have been facebook/react#11927 which affected react-dom v16.5.0.

Sorry that I could not help you more.

[jasco]

I should correct myself to say that the patch I referenced was applied to 16.5.0 rather than suggesting that the problem affected that version.

Just for testing purposes I confirmed that the onScroll synthetic event is triggered on a component within the shadow root in Chrome Version 79.0.3945.130 with react 16.12 after having removed the call to retarget. I haven't checked much beyond that and a couple of mouse events.

[treshugart]

This should be fixed by #12 and released in 0.6.0. Thanks everyone for the discussion and the fix!