Bug/486 nested navigation leads to crash

[svsk417]

Fixes #486

This cuts the dependencies of a child Beamer to its parent, if it is removed from the widget tree.

[codecov]

Codecov Report

Merging #490 (6799e54) into master (287e633) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 6799e54 differs from pull request most recent head 0b31e48. Consider uploading reports for the commit 0b31e48 to get more accurate results

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   96.71%   96.74%   +0.03%     
==========================================
  Files          13       13              
  Lines         853      861       +8     
==========================================
+ Hits          825      833       +8     
  Misses         28       28              

Impacted Files	Coverage Δ
package/lib/src/beamer.dart	92.15% <100.00%> (+0.66%)	⬆️
package/lib/src/beamer_delegate.dart	94.50% <100.00%> (+0.07%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 287e633...0b31e48. Read the comment docs.

[slovnicki]

This looks alright, although all of this nested navigation is a bit of a mess 🙂
Would it be possible for you to also add a test for this, but without using flutter_bloc like in #486?

[svsk417]

Hi @slovnicki! I just added the test ;). If executed with the current master branch, it crashes, while it does not with the applied changes. I am still not satisfied, that the _context is being cached within the beamer delegate, but this is another story ;)

[slovnicki]

@svsk417 Thanks!

I agree that playing with context like that is ugly and I'm open to change that. Let me tell you a short backstory how this came to be (recently).

We want to allow async guarding, but guards want context to execute their check. Previously, guards were only ran in build so it was ok and we didn't need to store context, but this was not great in case of some guarding loops (although this is problematic anywhere). To enable async guards, we need to move the guarding process out of build and into update. I had an idea to add this asyncCheck into the existing BeamGuard so I moved the entire process out of build and into update. Another option would be to use completely different guards (e.g. AsyncBeamGuard) for async checks, have them executed in update and move the execution of existing BeamGuards back into build only. Ah, an important thing to mention here; async guarding will not use context.

---

Also, maybe we could get context differently. I'm not sure why I didn't check for existence of navigatorKey.currentContext instead 🤔

[slovnicki]

Thanks for the PR! 💙