fix: wrong previousRoute after back journey #57

bchelkowski · 2023-11-30T12:52:05Z

What did you implement:

Closes #56

How did you implement it:

Save the latest item from kept history items instead of the current activeRoute.
It will also do that if the activeRoute should be skipped in the history.

How can we verify it:

The previousRoute should have the correct value always.

Todos:

Write documentation (if required)
Fix linting errors
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

RadoslawZambrowski

Nice and quick job @bchelkowski, well done 👏 thank you 🙇

Left some suggestions.

RadoslawZambrowski · 2023-11-30T13:03:55Z

src/components/router/Router.brs

+
+  if (isBackJourney OR m.top.activatedRoute.shouldSkip)
+    previousRoute = CreateObject("roSGNode", "ActivatedRoute")
+    previousRouteData = m._history[m._history.count() - 1]


Suggested change

previousRouteData = m._history[m._history.count() - 1]

previousRouteData = m._history.peek()

RadoslawZambrowski · 2023-11-30T13:11:10Z

src/components/router/Router.brs

+sub _deleteKeys(assocArray as Object, keys as Object)
+  for each key in keys
+    assocArray.delete(key)
+  end for
+end sub


that looks like a utility function that could be added to ObjectUtils, something like omit from lodash

I agree, but such a function does not exist yet, so I left a TODO comment.

We have ObjectUtils().pick()

I don't want to use the pick, as it will require you to modify it each time you would like to add something.

we need the opposite of ObjectUtils().pick() here 😅

RadoslawZambrowski · 2023-11-30T13:12:46Z

src/components/router/Router.brs

+  historyItem = route.getFields()
+  _deleteKeys(historyItem, ["change", "focusable", "focusedChild", "id"])
+
+  return historyItem


having the omit function added to the ObjectUtils we could simply do here as per below

Suggested change

historyItem = route.getFields()

_deleteKeys(historyItem, ["change", "focusable", "focusedChild", "id"])

return historyItem

return ObjectUtils().omit(route.getFields(), ["change", "focusable", "focusedChild", "id"])

RadoslawZambrowski · 2023-11-30T13:14:35Z

src/components/router/Router.brs

+    rootRoute = CreateObject("roSGNode", "ActivatedRoute")
+    rootRoute.setFields({ path: rootPath })
+
+    m._history.push(rootRoute.getFields())


_createHistoryItem seems to be missing

Suggested change

m._history.push(rootRoute.getFields())

m._history.push(_createHistoryItem(rootRoute.getFields()))

Not missing, but yes, it would be better to use _createHistoryItem.

Suggested change

m._history.push(rootRoute.getFields())

m._history.push(_createHistoryItem (rootRoute))

by missing I meant to keep the consistent interface of the objects stored in the history 😊

RadoslawZambrowski · 2023-11-30T13:15:44Z

src/components/router/Router.brs

+    rootRoute = CreateObject("roSGNode", "ActivatedRoute")
+    rootRoute.setFields({ path: rootPath })


we could add helper function like _createActivatedRoute(data) and use here and in _getPreviousRoute

RadoslawZambrowski · 2023-11-30T13:23:51Z

src/components/router/Router.brs

+function _getPreviousRoute(data as Object) as Object
+  isBackJourney = getProperty(data, "isBackJourney", false)
+
+  if (isBackJourney OR m.top.activatedRoute.shouldSkip)


I wonder if we really need to apply this logic when m.top.activatedRoute.shouldSkip is true. This is a different flag than skipInHistory and it is not set on back journey, but could be set by the middleware. Meaning that it is possible to skip routes only in the non-back journey (skipped routes are not in the history), and in that case previousRoute should stay as is.

E.g. we navigate from route A to route B, middleware updates route B to be skipped and redirects to route C, then we should not set route B as the previousRoute and route A should be still there. If we apply the below logic, then we would update the previousRoute with the route A, but it seems to be redundant as it is already there.

this is basically what I mean we could do:

Suggested change

if (isBackJourney OR m.top.activatedRoute.shouldSkip)

if (m.top.activatedRoute.shouldSkip)

return m.top.previousRoute

end if

if (isBackJourney)

now both solutions point to the same object, so I would just skip that additional check

pawelhertman

👌

pawelhertman · 2023-11-30T13:36:12Z

src/components/router/Router.brs

-
-  m.top.activatedRoute.path = data.path
-  m.top.activatedRoute.params = ternary(data.params <> Invalid, data.params, {})
+  ' Needs to be set before activatedRoute as setPreviousRoute uses the previous value of activatedRoute.


Suggested change

' Needs to be set before activatedRoute as setPreviousRoute uses the previous value of activatedRoute.

' Needs to be set before activatedRoute as getPreviousRoute uses the previous value of activatedRoute.

BTW this comment makes sense only if you put an empty line after setting m.top.previousRoute

pawelhertman · 2023-11-30T13:39:15Z

src/components/router/Router.brs

-sub _updateHistory()
-  if (m.top.url = "" OR m.top.activatedRoute.shouldSkip)
-    return
+function _getPreviousRoute(data as Object) as Object


It lacks context on what kind of data it is. You could pass here only isBackJourney, but it's still a bit strange why anything about the back journey should have an impact on the previous route. That's why I think the if (isBackJourney OR m.top.activatedRoute.shouldSkip) check should be part of the navigate method and this one should only return indeed the previous route

Changed to navigateData.
I don't agree, the function _getPreviousRoute is responsible for returning the previous route, so it shouldn't be shocking that it has all checks for that purpose.

I could argue that during navigation the current route is not the previous route until we switch it to the new one 😅

The output of the _getPreviousRoute is set to the m.top.previousRoute always.
So in my opinion whole logic should be there, even if previousRoute would be the current activatedRoute.

pawelhertman · 2023-11-30T13:42:34Z

src/components/router/Router.brs

+    previousRoute = CreateObject("roSGNode", "ActivatedRoute")
+    previousRouteData = m._history[m._history.count() - 1]
+
+    if (previousRouteData = Invalid) then return Invalid
+
+    previousRoute.setFields(previousRouteData)


Suggested change

previousRoute = CreateObject("roSGNode", "ActivatedRoute")

previousRouteData = m._history[m._history.count() - 1]

if (previousRouteData = Invalid) then return Invalid

previousRoute.setFields(previousRouteData)

previousRouteData = m._history[m._history.count() - 1]

if (previousRouteData = Invalid) then return Invalid

previousRoute = CreateObject("roSGNode", "ActivatedRoute")

previousRoute.setFields(previousRouteData)

changed that according to Radek's comment

pawelhertman · 2023-11-30T13:47:40Z

src/components/router/Router.brs

+  historyItem = route.getFields()
+  _deleteKeys(historyItem, ["change", "focusable", "focusedChild", "id"])
+
+  return historyItem


This is an interface change - for some purpose fields like renderedUrl or isBackJourney aren't stored (especially the renderedUrl makes no sense), so I think we shouldn't change this.

I believe it is needed to recreate properly the ActivatedRoute instance from the history item

How could we live without this then? 🤔 Anyway, makes sense 🙈

This is an interface change, but non-breaking, as it added fields that didn't exist before and didn't change the existing ones.
And it would be better in my opinion to keep all data related to the route instead of selecting some of it, we should not decide what would be useful to the user.

we were not recreating the ActivatedRoute from the history item before, it was introduced by Błazej within this PR 😄

So maybe we could just switch to storing the ActivatedRoute node? 🤔

RadoslawZambrowski · 2023-11-30T14:16:28Z

src/components/router/Router.brs

+function _getPreviousRoute(navigationData as Object) as Object
+  isBackJourney = getProperty(navigationData, "isBackJourney", false)


how about passing boolean parameter instead, ie isBackJourney? currently we use various naming for parameters across various functions, and it does not seem to be straightforward what are they. We have data, navigationData, and routeData 👀

Wouldn't it be even clearer?

if isBackJourney m.top.previousRoute = _getPreviousRoute() else m.top.previousRoute = NodeUtils().cloneNode(m.top.activatedRoute) end if

Anyway, no hard feelings, up to you @bchelkowski

and rename _getPreviousRoute to _getPreviousRouteFromTheHistory then

but we could still pass the boolean parameter to the _getPreviousRoute and apply proper logic inside as it is right now

sorry, but I don't understand why I should introduce yet another if statement in the navigate function if I could move it to the _getPreviousRoute

my idea was to extract the isBackJourney in the navigate function and pass it as a param, so the if stays in the _getPreviousRoute

in navigate:

m.top.previousRoute = _getPreviousRoute(getProperty(data, "isBackJourney", false))`

So like it is now?

## [2.1.5](v2.1.4...v2.1.5) (2023-12-05) ### Bug Fixes * wrong previousRoute after back journey ([#57](#57)) ([cdd6652](cdd6652))

github-actions · 2023-12-05T09:03:00Z

🎉 This PR is included in version 2.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: wrong previousRoute after back journey

85720dd

bchelkowski requested a review from a team as a code owner November 30, 2023 12:52

bchelkowski requested review from adamczopek and shirishapitta and removed request for a team November 30, 2023 12:52

bchelkowski mentioned this pull request Nov 30, 2023

Router's previousRoute field has wrong value after back journey #56

Closed

RadoslawZambrowski reviewed Nov 30, 2023

View reviewed changes

pawelhertman reviewed Nov 30, 2023

View reviewed changes

code review changes

1ef56e6

RadoslawZambrowski reviewed Nov 30, 2023

View reviewed changes

bchelkowski added 2 commits November 30, 2023 15:20

rename variables

fea8e2b

pass boolean to the _getPreviousRoute

a68896e

RadoslawZambrowski approved these changes Nov 30, 2023

View reviewed changes

pawelhertman approved these changes Dec 4, 2023

View reviewed changes

history will be an array of activatedRoutes now

6b8e5d6

bchelkowski force-pushed the fix--wrong-previousRoute-after-back-journey branch from b7d068e to 6b8e5d6 Compare December 5, 2023 09:00

bchelkowski merged commit cdd6652 into master Dec 5, 2023
1 check passed

bchelkowski deleted the fix--wrong-previousRoute-after-back-journey branch December 5, 2023 09:02

github-actions bot pushed a commit that referenced this pull request Dec 5, 2023

chore(release): 2.1.5 [skip ci]

1442398

## [2.1.5](v2.1.4...v2.1.5) (2023-12-05) ### Bug Fixes * wrong previousRoute after back journey ([#57](#57)) ([cdd6652](cdd6652))

github-actions bot added the released label Dec 5, 2023

	previousRouteData = m._history[m._history.count() - 1]
	previousRouteData = m._history.peek()

	m._history.push(rootRoute.getFields())
	m._history.push(_createHistoryItem(rootRoute.getFields()))

	m._history.push(rootRoute.getFields())
	m._history.push(_createHistoryItem (rootRoute))

		rootRoute = CreateObject("roSGNode", "ActivatedRoute")
		rootRoute.setFields({ path: rootPath })

-  if (isBackJourney OR m.top.activatedRoute.shouldSkip)
+  if (m.top.activatedRoute.shouldSkip)
+    return m.top.previousRoute
+  end if
+  if (isBackJourney)

	' Needs to be set before activatedRoute as setPreviousRoute uses the previous value of activatedRoute.
	' Needs to be set before activatedRoute as getPreviousRoute uses the previous value of activatedRoute.

		function _getPreviousRoute(navigationData as Object) as Object
		isBackJourney = getProperty(navigationData, "isBackJourney", false)

fix: wrong previousRoute after back journey #57

fix: wrong previousRoute after back journey #57

Conversation

bchelkowski commented Nov 30, 2023

What did you implement:

How did you implement it:

How can we verify it:

Todos:

RadoslawZambrowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RadoslawZambrowski Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelhertman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pawelhertman Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

RadoslawZambrowski Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023

RadoslawZambrowski Nov 30, 2023 •

edited

Loading

pawelhertman Nov 30, 2023 •

edited

Loading

RadoslawZambrowski Nov 30, 2023 •

edited

Loading