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

[fix] 'route/:id' refresh issue by removing explicit 'base' from vite… #140

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shellyear
Copy link
Collaborator

@shellyear shellyear commented Apr 13, 2024

@blcham solving #125

It works with dev and build. But not in the dockerization, working on it

@blcham
Copy link

blcham commented Apr 15, 2024

Seems like you are not returning config.js but index.html (i.e. config.js is search in different path)

@shellyear
Copy link
Collaborator Author

Seems like you are not returning config.js but index.html (i.e. config.js is search in different path)

Yes, the build(production) and dev need the "base" property in vite.config.js to be default "/", but the build for the dockerization should have "base" property as "/record-manager/", because of how it's set in docker-compose.

@shellyear
Copy link
Collaborator Author

shellyear commented Apr 18, 2024

@blcham Vite serves the app from the root path (/) by default for the dev and production (base: '/' in vite.config.js) . When build for production, Vite generates the assets with paths relative to the specified base.
The docker-compose.yml > record-manager > environment has a BASENAME custom environment variable which will be then used in config.js.template in .docker folder and written to config.js and then the BASENAME variable will be used in the source code for the react-router as it's property in App.js.
The base in vite.config.js and in the BASENAME should match https://v5.reactrouter.com/web/api/BrowserRouter/basename-string

Current state: works on dev, prod and dockerization

@shellyear shellyear requested a review from blcham April 18, 2024 16:50
@@ -3,7 +3,7 @@ RECORD_MANAGER_APP_TITLE=Record Manager
RECORD_MANAGER_APP_INFO=
RECORD_MANAGER_LANGUAGE=cs
RECORD_MANAGER_NAVIGATOR_LANGUAGE=true
RECORD_MANAGER_BASENAME=/
RECORD_MANAGER_BASENAME=/record-manager
Copy link

Choose a reason for hiding this comment

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

What is motivation to change this?

Copy link
Collaborator Author

@shellyear shellyear Apr 21, 2024

Choose a reason for hiding this comment

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

@blcham Because I thought of every environment (dev, production) having it's own base in vite.config.js which will use the value of the RECORD_MANAGER_BASENAME based on mode. So the dev environment will have a "/" base, and the production will have a "/record-manager" base . Since the build for production image is then used in dockerization, it means that the base of the build (in vite.config.js) should match with the BASENAME passed via docker (which will be /record-manager, since RECORD_MANAGER_ROOT_PATH in deploy > internal-auth > .env is commented out). https://reactrouter.com/en/main/router-components/router

Copy link

@blcham blcham Apr 21, 2024

Choose a reason for hiding this comment

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

So what if i decide to have RECORD_MANAGER_ROOT_PATH=/my-record-manager ?

Copy link
Collaborator Author

@shellyear shellyear Apr 22, 2024

Choose a reason for hiding this comment

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

@blcham It doesn't work after I changed it, I feel stuck now

deploy/internal-auth/docker-compose.yml Outdated Show resolved Hide resolved
const env = loadEnv(mode);

return defineConfig({
base: env.RECORD_MANAGER_BASENAME,
Copy link

Choose a reason for hiding this comment

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

I might not understand how it works, but

this seems to me strange to use. I thought we had environments set up in

export const BASENAME = getEnv("BASENAME", "");

So I am not sure how this setting is useful for us. I thought we would set Router basename instead.

Copy link

Choose a reason for hiding this comment

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

I assume (but did not check) vite.config.js base attribute is set during build time and thus is useless for us for dockerization.

@blcham
Copy link

blcham commented Apr 20, 2024

Current state: works on dev, prod and dockerization

I did not test the dockerization, but I have doubts about the solution. I have questions:

  1. do setting BASENAME really work in runtime? I.e. i should be possible to do only the following steps:
    • 1.1) after docker-compose up it should work on http://localhost:1235/record-manager
    • 1.2) change .env variable RECORD_MANAGER_ROOT_PATH=/record-manager-super-demo
    • 1.3) after docker-compose up it should work on http://localhost:1235/record-manager-super-demo
    • 1.4) then you can try also RECORD_MANAGER_ROOT_PATH=/testing-complex-path/record-manager-demo :)

@shellyear shellyear force-pushed the fix/route-navigation branch from d9935a7 to c0c7331 Compare April 21, 2024 12:46
@shellyear shellyear force-pushed the fix/route-navigation branch from c0c7331 to feec894 Compare April 25, 2024 13:38
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