Fix for query param issue with base url #2894

sachitv · 2021-03-04T04:49:36Z

This fixes #2887.

vdonato

Hi @sachitv, thanks so much for your contribution!

The general approach to this fix is correct, but there are a few details that I think need to be ironed out before this can be merged. Please see my comments below.

Additionally, it'd be great if you could add some unit tests for the handlePageInfoChanged method to App.test.tsx to make it less likely that a regression causes this issue to happen again. The tests should be added into a new App.handlePageInfoChanged describe block and will likely be structurally similar to the existing App.handleNewReport tests. We'll want to check four cases:

/ pathname with no query string
/ pathname with a query string
non-/ pathname with no query string
non-/ pathname with a query string

Feel free to ask me if you happen to have any questions about adding tests!

vdonato · 2021-03-07T04:11:56Z

frontend/src/App.tsx

Two things to note about these lines:

It's fine to use relative URLs with pushState, so I think we'll want to do that and thus drop document.location.origin here for conciseness.

This will always evaluate to ?${queryString} since by javascript operator precedence, this is equivalent to

(document.location.origin + document.location.pathname + queryString) ? `?${queryString}` : "/"

I believe we want to do something like

const targetUrl = document.location.pathname + (queryString ? `?${queryString}` : "")

where we can drop the / in the ternary operator as it'll come from pathname.

sachitv · 2021-03-08T06:26:20Z

Thanks for the feedback @vdonato . I’ll make the changes this week and add the unit tests as well.

stale · 2021-03-22T07:15:20Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sachitv · 2021-03-24T06:34:10Z

Sorry for the delay, I have been able to make your fixes locally. Though when it comes to creating the unit tests, I might need to specify additional configuration options for these tests namely:

 --server.baseUrlPath="foobar"

I noticed that there's no generic way to pass parameters for individual test cases. Would there be an objection to adding something like this?

I was thinking of creating special folder called 'extra_parameters' alongside scripts and specs in the e2e folder. This would basically contain some .txt files containing text like the above, which is read and passed as command line parameters to the streamlit invocation.

I was wondering if I could use a special toml file here instead? Thoughts @vdonato ?

sachitv · 2021-03-24T06:41:26Z

Alternatively I could just move my scripts into some sort of 'special' folder and handle the arguments on a case by case basis. Just looking for some guidance on what would be the best path forward.

vdonato · 2021-03-24T21:44:18Z

Hey @sachitv, no worries about the delay.

As far as testing goes, I think writing tests at the level where we're setting
config options may end up being a bit too much effort for the functionality
we're trying to validate here (it's just a few lines of code, so having to spin
up a streamlit server and pass config options to it + potentially restructure
our scripts so we're able to do so seems like overkill to me).

An approach that will be both easier and more practical given the size of the
function we're testing would be to write tests similar to those that can be
found in the App.handleNewReport describe block of App.test.tsx. The
general structure of these tests will probably look like

Change window.location.pathname to be a pathname we want to test.
(Note: we'll want to be sure to save the original value and restore it when
all of the tests have completed)
Initialize a PageInfo protobuf with the queryString we want to test. This
is similar to how a NewReport message gets created for the handleNewReport
tests.
Create an app (the same way as in the handleNewReport tests)
Call the handlePageInfoChanged method with the PageInfo proto created
above
Verify that window.history.pushState was called with the arguments we expect.
You may have to read up a bit on how jest.spyOn
works for this if you're not familiar with it. Conveniently there are already
some usage examples in App.test.tsx.

I listed out some of the cases we want to test in a previous comment.

sachitv · 2021-03-25T06:07:44Z

Thanks for the feedback @vdonato , please can you have a look at these changes? Given i'm not really a TypeScript programmer, I apologize in advance if this freaks you out.

vdonato

Hey @sachitv, this looks good to me! I have a few small stylistic points to bring up in the tests, but they're all minor.

Once those are fixed, I can get a second reviewer to take a look (we generally require two people to look at community contributions), and we'll be able to get these changes merged.

frontend/src/App.test.tsx

This fixes streamlit#2887. Also adds unit tests to App.test.tsx.

sachitv · 2021-03-26T02:18:01Z

Hi Vincent, Appreciate the feedback. I've addressed all your requested changes. Hopefully in a good state to add a second reviewer now?

vdonato

Looks good to me, thanks for your work on this @sachitv!

I'll ping people to have a second reviewer take a look on Monday (there's a company hackathon going on right now so I'd imagine everyone is heads down on their projects 🙂)

sachitv · 2021-03-26T03:40:52Z

Looks good to me, thanks for your work on this @sachitv!

My pleasure.

I'll ping people to have a second reviewer take a look on Monday (there's a company hackathon going on right now so I'd imagine everyone is heads down on their projects 🙂)

Nice, have fun!

Btw how does the release process work? Would it be possible to get a minor version release after this merged or would we have to wait for the next scheduled release? It would be really nice to have these fixes available for my work project.

vdonato · 2021-03-26T19:26:54Z

Btw how does the release process work? Would it be possible to get a minor version release after this merged or would we have to wait for the next scheduled release? It would be really nice to have these fixes available for my work project.

We do occasionally do patch releases for issues that sneak in that are especially bad, but I think for an issue of this severity being fixed, it'll be included in the next scheduled release on April 8 (there don't seem to be too many people running into #2887).

Note that if you really need it earlier and are okay with dealing with the potential for a bit of instability, we auto-publish a streamlit-nightly package that'll include this change the next time it's built after this PR is merged.

sachitv · 2021-03-28T02:50:21Z

Thanks for the response. I think I can wait until April 8 for the next scheduled release.

ghost

Looks good to me!

sachitv · 2021-03-29T22:52:57Z

Thanks @bh-streamlit and @vdonato for the review. Unfortunately I don't have the bits to merge this. Please could you hit the green button for me?

FYI this is what I see:

vdonato · 2021-03-29T22:56:32Z

@sachitv merged! Thanks again for your work!

* develop: Set baseUrl to . Speed up CircleCI more (#3025) Add events to keep track of theme changes and other theme stats (#3012) Have MetricsManager enqueue events when disconnected (#3011) Fix for query param issue with base url (#2894) Add fuzzy search to selectbox (#2933) Informative repr methods for our python classes (#3027) ComponentInstance: handle iframe changes (#3015)

* st_form: Set baseUrl to . Speed up CircleCI more (streamlit#3025) Add events to keep track of theme changes and other theme stats (streamlit#3012) Have MetricsManager enqueue events when disconnected (streamlit#3011) Fix for query param issue with base url (streamlit#2894) Add fuzzy search to selectbox (streamlit#2933) Informative repr methods for our python classes (streamlit#3027) ComponentInstance: handle iframe changes (streamlit#3015)

sachitv requested a review from a team March 4, 2021 04:49

vdonato requested changes Mar 7, 2021

View reviewed changes

stale bot added the stale label Mar 22, 2021

stale bot closed this Mar 23, 2021

vdonato reopened this Mar 24, 2021

stale bot removed the stale label Mar 24, 2021

sachitv force-pushed the fix_url_path_issue branch from 5d1dc1c to c0bdaa7 Compare March 25, 2021 06:04

sachitv force-pushed the fix_url_path_issue branch 2 times, most recently from f4bceef to b3375f9 Compare March 25, 2021 06:36

vdonato reviewed Mar 25, 2021

View reviewed changes

Fix for query param issue with base url

59288c7

This fixes streamlit#2887. Also adds unit tests to App.test.tsx.

sachitv force-pushed the fix_url_path_issue branch from b3375f9 to 59288c7 Compare March 26, 2021 02:16

vdonato reviewed Mar 26, 2021

View reviewed changes

vdonato approved these changes Mar 26, 2021

View reviewed changes

ghost approved these changes Mar 29, 2021

View reviewed changes

vdonato merged commit f299858 into streamlit:develop Mar 29, 2021

sachitv deleted the fix_url_path_issue branch March 29, 2021 23:00

Fix for query param issue with base url #2894

Fix for query param issue with base url #2894

Uh oh!

Conversation

sachitv commented Mar 4, 2021

Uh oh!

vdonato left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdonato Mar 7, 2021

Choose a reason for hiding this comment

Uh oh!

sachitv commented Mar 8, 2021

Uh oh!

stale bot commented Mar 22, 2021

Uh oh!

sachitv commented Mar 24, 2021

Uh oh!

sachitv commented Mar 24, 2021

Uh oh!

vdonato commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sachitv commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdonato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sachitv commented Mar 26, 2021

Uh oh!

vdonato left a comment

Choose a reason for hiding this comment

Uh oh!

sachitv commented Mar 26, 2021

Uh oh!

vdonato commented Mar 26, 2021

Uh oh!

sachitv commented Mar 28, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

sachitv commented Mar 29, 2021

Uh oh!

vdonato commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vdonato left a comment •

edited

Loading

vdonato commented Mar 24, 2021 •

edited

Loading

sachitv commented Mar 25, 2021 •

edited

Loading