Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add React router tests #1

Open
wants to merge 6 commits into
base: fix-for-jupyterlab
Choose a base branch
from
Open

Conversation

gabalafou
Copy link
Owner

This PR contains the tests for conda-incubator#429.

It is a separate PR because it makes significant changes to the React app code.

I believe that these changes are not only necessary to enable these tests but that they also fix a long-standing bug that I guess we did not even realize that we had.

Description

This pull request:

  • Swaps the order in which config variables are read. First it reads from condaStoreConfig, then from process.env (environment variables). Important note: these environment variables can only be read at build time.
  • Adds a new Playwright test file for router testing
  • Adds an HTML file that allows Playwright to load the app with memory routing configured
  • Configures Webpack to output the HTML file

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?


export const prefDefault: Readonly<IPreferences> = {
apiUrl:
process.env.REACT_APP_API_URL ??
condaStoreConfig.REACT_APP_API_URL ??
Copy link
Owner Author

@gabalafou gabalafou Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Conda Store docs say that condaStoreConfig can be used to configure the UI app at runtime.

I take this to also imply that run-time should override build-time configuration.

If not, it is impossible to set a config variable in the way that I do in the memory-router-test.html file in this PR if that variable was already defined as an environment variable at build time.

So for example, if in my .env file I have:

REACT_APP_ROUTER_TYPE=browser

And then in my app index html page I have:

<script>
window.condaStoreConfig = {
  REACT_APP_ROUTER_TYPE: "memory"
};
<script>

Then the app will actually be loaded with the browser router and not the memory router. I don't think that's what we want, but I may be mistaken.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtime indeed overwrites/supersedes build time.
This is now how we are passing default config values to conda-store-server vs the hack we used to have before https://github.com/conda-incubator/conda-store/blob/d39e5b47577aa6764b1bf99a0ab675fe8782e969/conda-store-server/conda_store_server/_internal/server/templates/conda-store-ui.html#L11

Copy link
Owner Author

@gabalafou gabalafou Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting. Does that file you sent actually work? Because as far as I can tell, the only way it would work is if the main.js file at static/conda-store-ui/main.js were built in an environment in which none of those REACT_APP_-prefixed variables were defined at build time. I searched the UI codebase for condaStoreConfig to see if I was missing something, but the only place that variable is read is in the preferences.tsx file, and in that file, it's pretty clear that the variable from condaStoreConfig is only read if the corresponding environment variable were nullish at build-time.

For example, let's look at the following JavaScript:

process.env.REACT_APP_API_URL ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

At build time Webpack will do a string substitution and replace process.env.REACT_APP_API_URL with the value in the environment, if it's defined. So, if it's set to "http://localhost:8080/conda-store/", then the above JavaScript is transformed to:

"http://localhost:8080/conda-store/" ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

Since the first argument in the above expression is not "nullish", meaning neither null nor undefined, then the overall expression reduces to that first argument, i.e.:

"http://localhost:8080/conda-store/"

On the other hand, if that environment variable is not defined at build time, the source code is transformed as:

undefined ??
condaStoreConfig.REACT_APP_API_URL ??
"http://localhost:8080/conda-store/"

Since the left hand side of the nullish operator is undefined the runtime will try to read the value from condaStoreConfig, and only if that too is null or undefined, will it return the last operand.

So the only way the condaStoreConfig variable plays any role at run time is if the corresponding environment variable was not defined at build time.

Comment on lines +44 to +50
def test_memory_router_404_not_found(page: Page, test_config):
"""The memory router has been configured to load the root view at any route
"""
# The route `/memory-router-test.html` is not a route recognized by the
# React app. With the browser router, an unknown route would give a 404.
page.goto(test_config["base_url"] + "/memory-router-test.html")
expect(page.get_by_test_id("no-environment-selected")).to_be_visible()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, do non-existing routes always redirect to /conda-store?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly. The memory router does not redirect anywhere. It loads the app at the route you pre-configure. For example, in my other PR, I configure the memory router to start with the root route. That means that no matter what the URL in the browser address bar says, if the app is loaded at that URL, it will load the root view.

The browser router, by contrast, tries to read the URL path from the browser address bar when the app loads and then render the app view that matches that route (or render a 404 if no view matches the route).

In other words, the browser router is URL-aware, whereas the memory router is not. That's the main difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants