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

Uncaught TypeError: Cannot read property 'insertBefore' of null #2609

Closed
benstaker opened this Issue Jul 5, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@benstaker

benstaker commented Jul 5, 2018

IMPORTANT

Your issue might be labeled as invalid and eventually closed if:

  • You have deleted completely the issue template for no reason
  • You can not reproduce the issue
  • Your question is not an issue nor a feature request but it's only a support/consulting request (use stack overflow for it)
  • Your post violates our code of conduct

Help us to manage our issues by answering the following:

  1. Describe your issue:

When Riot is trying to render an if={expression} it is being met with a null this.stub.parentNode and cannot call insertBefore on it.

  1. Can you reproduce the issue?

Post the link using one of our bug report templates:

Only in our codebase. Below is the template for a particular integration we have:

<widget-container-main
  if="{onPageProductEnabled}"
  on-mount="{onMainMount}"
  on-ready="{onMainReady}"
  mount-config="{mountConfig}">
  <div if="{widgetReady}"><!-- fails here -->
    <div class="widget-on-page-small-logo-container">
      <widget-logo-small widget-reference="{widgetReference}"></widget-logo-small>
    </div>
    <widget-global
      integration-type="onpage-product"
      widget-reference="{widgetReference}"></widget-global>
  </div>
</widget-container-main>

onPageProductEnabled by default is true.
onMainMount is a function that is called once widget-container-main has mounted, passing back event references to be used for namespacing and setting up event listeners.
onMainReady is a function that is called once the widget-container-main is ready (fetched all data from APIs), which then inserts a logo into the page.
mountConfig is an object containing any opts and default values from the riot tag for a particular integration.

one of the event listeners we have updates onPageProductEnabled again once it has fetched some data, such as amount of products. If this integration has no products then it sets it to false.

Because onPageProductEnabled is being set to false it then causes the element to be removed from the DOM... but it doesn't seem to be stopping any child if={} expressions from being run. I.e. the <div if="{widgetReady}">.

It seems like there may need to be some more checks before running insertBefore, or the ability to prevent child expressions from running if the parent element has been removed from the DOM.

  1. On which browser/OS does the issue appear?

Windows 10 64-bit, Google Chrome v67.0.3396.99 64-bit

  1. Which version of Riot does it affect?

Riot v3.10.3

  1. How would you tag this issue?
  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@benstaker

This comment has been minimized.

benstaker commented Jul 5, 2018

The last known working version was v3.7.3. I am currently downgrading from v3.10.3 until I can find a working version so that it can give you an idea of when this issue was introduced.

@benstaker

This comment has been minimized.

benstaker commented Jul 5, 2018

I have tried every single version between v3.7.3 and v3.10.3 and funnily enough it starts breaking from v3.7.4.

@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Jul 5, 2018

please create a live example that we could test. Right now for me it's impossible to verify the issue. Thank you

@benstaker

This comment has been minimized.

benstaker commented Jul 6, 2018

This was very difficult to replicate, but I have finally managed to do it. Here's the plunker:
https://plnkr.co/edit/FQSpGrwx0mVxFpECBkB2?p=preview

@jobrodesign

This comment has been minimized.

jobrodesign commented Jul 6, 2018

Going to take a stab at answering this.

I see the issue you're referencing where an if expression is evaluated even when the tag has been removed from the DOM, but I dont think the fix should be a riot fix. Your example only produces an error because of the random update initiated from within the removed child tag, not the update initiated from the parent in the simulated onDataFetched. It does not appear that riot is allowing evaluation of if expressions on removed child tags when the update is initiated from the parent.

Removing the random child tag initiated update avoids the error as seen is this plunker:
https://plnkr.co/edit/B6ztTKifJLWdpyDS6a6S?p=preview

Riot seems to be working as designed.

@benstaker

This comment has been minimized.

benstaker commented Jul 6, 2018

I have updated the plunker to use a series setTimeout's vs. setInterval, to simulate asynchronous code such as event listeners and/or API requests:
https://plnkr.co/edit/FQSpGrwx0mVxFpECBkB2

This only starts to error after 500ms, which is when the tag.opts.onReady callback is called, causing the child component to unmount via the if={isReady}.

Suggestion: There should be a tag.isMounted check before running the tag.update code, or alternatively add a new flag/state for when the tag has been destroyed or unmounted. Then you can do !tag.isUnmounted before running the tag.update code.

@jobrodesign

This comment has been minimized.

jobrodesign commented Jul 6, 2018

You could make those checks before running update in the child tag (I think that's possible now without a new built-in state), but its unnecessary. Your tests keep initiating the update from the CHILD tag but that isn't consistent with your use case which is an update as the result of new data fetched at the PARENT tag level. The updates that originate from the parent tag do not result in errors.

Put another way, if your child tags were themselves making some asynch call AND there was some outside value that could cause the child tag to be removed from the DOM in between request and response, then yes the if would be evaluated and throw the error. But even in this scenario, you could easily avoid this by cancelling the asynch request using the built in unmount event at the child tag level.

But the above isnt your scenario. The parent tag gets data and updates accordingly

@zorgoz

This comment has been minimized.

zorgoz commented Jul 10, 2018

I have the same issue without firing update directly from behind an if. I can't simply reproduce it, but here is the scenario:

  1. Button clicked behind if put on a div for example
  2. Event handler calls the server over SignalR
  3. Server triggers client script over SignalR passing some data
  4. Client script triggers a Riot Observable (message bus) passing that data
  5. Subscribed tag method modifies members using the data passed and calls this.update()
  6. That if evaluates to false because of the new data

Observations:

  • no Riot tag gets unmounted meanwhile
  • there are no child tags either that would be affected by this change
  • looks not to happen under all circumstances
  • this is not a single call chain, as 1-2 and 3-4-5-6 are two different paths
  • show= always works
@GianlucaGuarini

This comment has been minimized.

Member

GianlucaGuarini commented Jul 12, 2018

I have fixed the issue https://plnkr.co/edit/R3ppRDzISMpGhpL76Jd4?p=preview . I will publish a new riot release during the weekend. Thanks for reporting it

@benstaker

This comment has been minimized.

benstaker commented Jul 13, 2018

@GianlucaGuarini thanks so much! Once released I will test it with our code and confirm it is working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment