Skip to content

Commit

Permalink
Bugfix: Multiple Stale Maps (streamlit#9092)
Browse files Browse the repository at this point in the history
## Describe your changes

So debugging this took me all over the codebase. 😅 But one thing that
kept popping up that was giving me a gut reaction that it might be the
culprit was the duplicate key React errors we're seeing in console in
`develop`. I tested if making the id's unique fixed the problem, and it
did. I then read the comment above this function `/** Return an
Element's widget ID if it's a widget, and undefined otherwise. */` So I
then changed it to if it is a map, return `undefined`. If that comment
is indeed true, should we make this change apply to all non-widgets?
(And if so, how does one check if an element is a widget? Is there some
helper somewhere for that?)


| Before | After |
| ------ | ------|
| ![Kapture 2024-07-16 at 19 37
19](https://github.com/user-attachments/assets/7c6ed82d-7037-423b-905e-f1a0687c4a38)
| ![Kapture 2024-07-16 at 19 28
15](https://github.com/user-attachments/assets/bb977290-ff4b-433c-80c5-e3bc7e976824)
|

## GitHub Issue Link (if applicable)

This PR fixes streamlit#8329

## Testing Plan

Manually tested as seen in above screen captures. If the team is aligned
with the fix, I'd like to add some automated test coverage for this.


# Update Post Discussion
The engineering team met to discuss a few options to resolve this bug
considering future plans in this area.

1. Renaming the proto field from `id` to `hash`
2. Compute a proper widget id for `st.map` and add a `key` argument to
`st.map`.
3. Add element differentiation to the frontend code (if `st.map` then
ignore `id`)

Given that we will soon be workling on selections in maps, we decided to
go with the easiest fix as a short term solution, option 3. We plan to
remove this code when we properly fix it.

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
  • Loading branch information
sfc-gh-nbellante authored Jul 26, 2024
1 parent 15e17fe commit 96a1e82
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 0 deletions.
43 changes: 43 additions & 0 deletions e2e_playwright/st_map_ensure_no_stale_maps.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Copyright (c) Streamlit Inc. (2018-2022) Snowflake Inc. (2022-2024)
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import numpy as np
import pandas as pd

import streamlit as st

st.write(f"Streamlit version = {st.__version__}")

df1 = pd.DataFrame(
np.random.randn(10, 2) / [50, 50] + [37.76, -122.4], columns=["lat", "lon"]
)

df2 = pd.DataFrame(
np.random.randn(10, 2) / [50, 50] + [-37.76, 122.4], columns=["lat", "lon"]
)

option = st.selectbox("which dataframe to use?", ("1", "2"))

st.write("You selected:", option)

df = df1 if option == "1" else df2

st.map(df)
st.write(df)

st.write("df1")
st.map(df1)

st.write("2")
st.map(df2)
35 changes: 35 additions & 0 deletions e2e_playwright/st_map_ensure_no_stale_maps_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright (c) Streamlit Inc. (2018-2022) Snowflake Inc. (2022-2024)
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from playwright.sync_api import Page, expect


def test_st_map_has_no_stale_elements(
themed_app: Page,
):
maps = themed_app.get_by_test_id("stDeckGlJsonChart")
expect(maps).to_have_count(3)

selectbox = themed_app.get_by_test_id("stSelectbox").first
selectbox.locator("input").first.click()
selection_dropdown = themed_app.locator('[data-baseweb="popover"]').first
selection_dropdown.locator("li").nth(1).click()

expect(maps).to_have_count(3)

selectbox.locator("input").first.click()
selection_dropdown = themed_app.locator('[data-baseweb="popover"]').first
selection_dropdown.locator("li").nth(0).click()

expect(maps).to_have_count(3)
6 changes: 6 additions & 0 deletions frontend/lib/src/util/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,12 @@ export function setCookie(

/** Return an Element's widget ID if it's a widget, and undefined otherwise. */
export function getElementWidgetID(element: Element): string | undefined {
// NOTE: This is a temporary fix until the selections in maps work is done.
// We believe that this will be easier to fix when we get to that point so in
// the meantime we will be doing this simple fix to prevent this error: https://github.com/streamlit/streamlit/issues/8329
if (notNull(element.deckGlJsonChart)) {
return undefined
}
return get(element as any, [requireNonNull(element.type), "id"])
}

Expand Down

0 comments on commit 96a1e82

Please sign in to comment.