bug: fix AppRouting interference with URL queryParam-reading components #4213
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
psybuzz
requested review from
stephanwlee and
wchargin
and removed request for
stephanwlee and
wchargin
stephanwlee
approved these changes
Oct 2, 2020
Comment on lines
21
to
22
| * Ideally, AppRouting would manage all URL params. | ||
| * https://github.com/tensorflow/tensorboard/issues/4207 |
Contributor
There was a problem hiding this comment.
You can drop this line. Even if router gains the capability, we want this component to read from the URL to bootstrap certain feature flags states.
Contributor
Author
|
As per offline discussion, in a followup CL, I will write a regression test that checks that our app doesn't emit POST requests under ?tensorboardColab=true. |
psybuzz
added a commit
that referenced
this pull request
Oct 12, 2020
AppRoutingEffects recently added logic to preserve URL hash upon the initial 'navigated' action (see [1]). While this fixed cases where the URL needs to be read upon initial page load, it does not address an upcoming case needed for deeplinking pins in Time Series. Flow: pin a card, DeepLinkProvider serializes `?pinnedCards=` in the URL which drops `#timeseries`, reload the tab, the selected dashboard is no longer Time Series. Expected: Time Series remains selected upon tab reload. This change overcomes this issue the DeepLinkProvider by preserving the hash upon same-RouteId navigations. Manually checked internal TB embedders that navigating to a different route id still clears the hash. Googlers, see sync cl/335688801 Followup: #4220 [1] #4213
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #4207 for details.
Several callsites in TB's Angular and Polymer codebases read
directly from
window.location.search. For example, scalar cardsread the
?tensorboardColab=trueparam at arbitrary times inthe TB session, to know whether TB is running inside Colab.
AppRoutingEffects has an initialization step that clears/discards
all existing URL queryParams, causing breaking behavior.
Since AppRouting is not yet a complete source of truth for URL
query params, this change makes all callsites that read from
window.location.searchdo so on the initial app load, beforeAppRouting can interfere.
An alternative considered was making AppRoutingEffects aware
of a hardcoded list of reserved queryParam keys, and preserving
them whenever updating the URL. In that approach, there are
some unknown questions, e.g. if a user navigates to a different
route and returns to the original route, do we still need to preserve
queryParams?
Manually tested that running TB with
?tensorboardColab=trueand opening scalar cards results in network requests to
the
/scalarsendpoint, not/scalars_multirun.