chore: Set router basename to simplify paths in /ui

[peruukki]

What this PR does / why we need it:

Sets the React Router basename to avoid repeating the PUBLIC_URL environment variable.

Which issue(s) this PR fixes:

This is pretty much a re-do of #3541 but with only internal changes. Related short discussion started in #3514 (comment).

Misc

I verified that the environment variable still works by starting the app with PUBLIC_URL=/feast-ui yarn start (also PUBLIC_URL=http://localhost:3000/feast-ui yarn start somehow works):

Note: I originally included some changes that affected the behavior (see discussion below), but eventually decided to keep this as purely refactoring, and make other kinds of improvements separately.

[HaoXuAI]

This is a really nice feature, thx!

[redhatHameed]

just checking is there anything required for updating the readme document for this change ?

[HaoXuAI]

yeah might be better to write something here: https://docs.feast.dev/reference/alpha-web-ui

[redhatHameed]

@peruukki can you see if any document needs update for this change-> https://github.com/feast-dev/feast/blob/master/docs/reference/alpha-web-ui.md

[peruukki]

I added a subsection about the new prop, see it rendered in alpha-web-ui.md and ui/README.md. (I also did tiny other tweaks.)

[peruukki]

It now occurred to me that this is actually a breaking change if you were using Feast UI as a module in your app and using the PUBLIC_URL environment variable: now you need to explicitly pass its value in the basename prop like we do in ui/src/index.tsx since the FeastUI component no longer references the environment variable.

So maybe that should be mentioned in the release notes? Let me know if you want me to change e.g. the PR title prefix from "chore" to something else due to this.

[HaoXuAI]

erences the environment variable.

So maybe that should be mentioned in the release notes? Let me know if you wan

Is there a way to make PUBLIC_URL still works in environment variables for feast?

[franciscojavierarceo]

hold

[franciscojavierarceo]

erences the environment variable.
So maybe that should be mentioned in the release notes? Let me know if you wan

Is there a way to make PUBLIC_URL still works in environment variables for feast?

+1 @peruukki any chance we can investigate this?

[peruukki]

To clarify, this doesn't remove the possibility to use PUBLIC_URL. But if you are using Feast UI as a module, you now need to pass that value to the FeastUI component via its basename prop.

If you were only defining the PUBLIC_URL environment variable to use it inside the FeastUI component, you can now remove it. If you want to keep using it, you'll need to make a change like this in your index.tsx file (similarly to the ui/src/index.tsx changes in this PR):

@@ -3,16 +3,19 @@ import ReactDOM from 'react-dom/client';
 
 import './index.css';
 import FeastUI from '@feast-dev/feast-ui';
 import '@feast-dev/feast-ui/dist/feast-ui.css';
 
+const basename = process.env.PUBLIC_URL ?? '';
+
 const root = ReactDOM.createRoot(document.getElementById('root')!);
 root.render(
   <StrictMode>
     <FeastUI
+      basename={basename}
       feastUIConfigs={{
-        projectListPromise: fetch(`${process.env.PUBLIC_URL ?? ''}/projects-list.json`, {
+        projectListPromise: fetch(`${basename}/projects-list.json`, {
           headers: {
             'Content-Type': 'application/json',
           },
         }).then((res) => {
           return res.json();

(And now looking at that, we could use basename in defaultProjectListPromise too, so you wouldn't always need to provide a custom projectListPromise just for this, but maybe that's for another pull request. 😄)

I would prefer not to use environment variables inside the FeastUI component because they are quite a hidden way to pass values and give you less flexibility on how you define the value, and I don't see why we couldn't use a regular prop here. But I also understand that we may want to preserve backwards compatibility. In that case, we can either:

1. Not add the basename prop to FeastUI, and we keep referencing PUBLIC_URL inside it. We can still keep the benefits of referencing it only in one place and in a way that doesn't cause an error if you haven't defined a process.env global; these were my main motivation for these changes anyway.
2. Have two ways of setting the basename, either via this new prop or via the PUBLIC_URL environment variable like before. And if both are defined, I would give the prop precedence since it's more local.

[HaoXuAI]

To clarify, this doesn't remove the possibility to use PUBLIC_URL. But if you are using Feast UI as a module, you now need to pass that value to the FeastUI component via its basename prop.

If you were only defining the PUBLIC_URL environment variable to use it inside the FeastUI component, you can now remove it. If you want to keep using it, you'll need to make a change like this in your index.tsx file (similarly to the ui/src/index.tsx changes in this PR):

@@ -3,16 +3,19 @@ import ReactDOM from 'react-dom/client';
 
 import './index.css';
 import FeastUI from '@feast-dev/feast-ui';
 import '@feast-dev/feast-ui/dist/feast-ui.css';
 
+const basename = process.env.PUBLIC_URL ?? '';
+
 const root = ReactDOM.createRoot(document.getElementById('root')!);
 root.render(
   <StrictMode>
     <FeastUI
+      basename={basename}
       feastUIConfigs={{
-        projectListPromise: fetch(`${process.env.PUBLIC_URL ?? ''}/projects-list.json`, {
+        projectListPromise: fetch(`${basename}/projects-list.json`, {
           headers: {
             'Content-Type': 'application/json',
           },
         }).then((res) => {
           return res.json();

(And now looking at that, we could use basename in defaultProjectListPromise too, so you wouldn't always need to provide a custom projectListPromise just for this, but maybe that's for another pull request. 😄)

I would prefer not to use environment variables inside the FeastUI component because they are quite a hidden way to pass values and give you less flexibility on how you define the value, and I don't see why we couldn't use a regular prop here. But I also understand that we may want to preserve backwards compatibility. In that case, we can either:

1. Not add the basename prop to FeastUI, and we keep referencing PUBLIC_URL inside it. We can still keep the benefits of referencing it only in one place and in a way that doesn't cause an error if you haven't defined a process.env global; these were my main motivation for these changes anyway.
2. Have two ways of setting the basename, either via this new prop or via the PUBLIC_URL environment variable like before. And if both are defined, I would give the prop precedence since it's more local.

ok I think that make sense. I like this feature because when I setup Superset it is very hard to have a ui prefix in the public URL. it would be convenient to have process.env.PUBLIC_URL works so that users can customize the web url.

[HaoXuAI]

To clarify, this doesn't remove the possibility to use PUBLIC_URL. But if you are using Feast UI as a module, you now need to pass that value to the FeastUI component via its basename prop.
If you were only defining the PUBLIC_URL environment variable to use it inside the FeastUI component, you can now remove it. If you want to keep using it, you'll need to make a change like this in your index.tsx file (similarly to the ui/src/index.tsx changes in this PR):

@@ -3,16 +3,19 @@ import ReactDOM from 'react-dom/client';
 
 import './index.css';
 import FeastUI from '@feast-dev/feast-ui';
 import '@feast-dev/feast-ui/dist/feast-ui.css';
 
+const basename = process.env.PUBLIC_URL ?? '';
+
 const root = ReactDOM.createRoot(document.getElementById('root')!);
 root.render(
   <StrictMode>
     <FeastUI
+      basename={basename}
       feastUIConfigs={{
-        projectListPromise: fetch(`${process.env.PUBLIC_URL ?? ''}/projects-list.json`, {
+        projectListPromise: fetch(`${basename}/projects-list.json`, {
           headers: {
             'Content-Type': 'application/json',
           },
         }).then((res) => {
           return res.json();

(And now looking at that, we could use basename in defaultProjectListPromise too, so you wouldn't always need to provide a custom projectListPromise just for this, but maybe that's for another pull request. 😄)
I would prefer not to use environment variables inside the FeastUI component because they are quite a hidden way to pass values and give you less flexibility on how you define the value, and I don't see why we couldn't use a regular prop here. But I also understand that we may want to preserve backwards compatibility. In that case, we can either:

1. Not add the basename prop to FeastUI, and we keep referencing PUBLIC_URL inside it. We can still keep the benefits of referencing it only in one place and in a way that doesn't cause an error if you haven't defined a process.env global; these were my main motivation for these changes anyway.
2. Have two ways of setting the basename, either via this new prop or via the PUBLIC_URL environment variable like before. And if both are defined, I would give the prop precedence since it's more local.

ok I think that make sense. I like this feature because when I setup Superset it is very hard to have a ui prefix in the public URL. it would be convenient to have process.env.PUBLIC_URL works so that users can customize the web url.

@franciscojavierarceo seems good to go.

[peruukki]

@franciscojavierarceo I reduced the scope of this PR and made it purely refactoring, i.e. no externally visible changes. It's better to make such changes separately after this.