Basename feature in #3722 doesn't work after "yarn build"

[meggarr]

Describe the bug
The feature in #3722 breaks the "NavLink" with "newTab". The package's homepage (aka basename) doesn't work after "yarn build".

This works on v3.15.0 tag, shall we revert ?

@haricane8133

Details
Conductor UI on the latest main.

To Reproduce
Steps to reproduce the behavior:

1. Build and run the conductor UI
2. Goto Workflow Definitions
3. Click the "Executions" 's Query Link,
4. It doesn't work.

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

[haricane8133]

@meggarr, could you check if this is because of https://github.com/Netflix/conductor/pull/3873/files#diff-c868321113245d9cb0c0ed91ae213c8a2b0be4f4ae257aefcef39bee60292c7eL29-R29?

@nhandt2021, we have the function cleanDuplicateSlash() right?
Do we have to remove the slash there? I think that removing that slash might have broken (I am unable to debug now)
Could you check and let me know if I need to assist?

[meggarr]

@haricane8133 No, it's not. That's because, at runtime, there isn't packageJson.hostname, so that's always empty/null. So the "newTab" of NavLink fails without host name, so it fails.

[nhandt2021]

@meggarr That's right
@haricane8133 the current regex doesn't check the duplicated slash right after the host name

[haricane8133]

@meggarr, react-scripts uses WebPack internally right? It is strange that it isn't bundling the JSON dependency...

@nhandt2021, I see. Maybe we have to change the comment/function name then.

[haricane8133]

The homepage field passed in package.json is available in the final JS...

[nhandt2021]

@meggarr
cc: @haricane8133
I've double checked again. It still works fine.

You need to run this command after building: yarn serve-build or npm run serve-build (https://github.com/conductor-oss/conductor/blob/main/ui/package.json#L51)

[haricane8133]

@haricane8133 the current regex doesn't check the duplicated slash right after the host name

@nhandt2021, the regex works at all places except at ://

[meggarr]

@nhandt2021 What about docker build, does that work ?

[nhandt2021]

@haricane8133 the current regex doesn't check the duplicated slash right after the host name

@nhandt2021, the regex works at all places except at ://

@haricane8133 doesn't work (for below case), that's why I raised that PR

[haricane8133]

Yeah. I just found that too. Looking for a regex that can accommodate this...

[nhandt2021]

@nhandt2021 What about docker build, does that work ?

@meggarr It also works for me
Actually @haricane8133 provide a way to config a custom host name easier. If you don't change anything relate to the host path. I think it should work as usual with the default settings 😃

The below image, system couldn't fetch the API because I built UI image only. But look at the API's path, it's normal.
Btw, I think it's not relate to your issue because the UI don't run as a built version, it run as development inside the container.

[haricane8133]

@nhandt2021, there are two parts here

1. react-scripts needs to know the correct path to JS and CSS, to be set inside the generated HTML.
2. Router needs to know how to handle in-browser navigation
3. ... (check this issue for more details - [FEATURE]: Conductor UI's Static App must be easily hostable from any base route Netflix/conductor#3656)

So, setting homepage inside package.json is the easiest way, as that field is anyway required by react-scripts to set the paths to JS and CSS inside the generated HTML. My changes would reuse the same variable for other places where the change must come in (like router, NavLink, etc)

[haricane8133]

1. There is an issue with newTab navigation as @meggarr mentioned, and the issue is related to the regex not catching multiple slashes at the start of the string.
2. We also need the + '/' + I added as a part of my PR. The reason for that is, the homepage field can either contain a trailing /, or NOT. This was removed with this PR.

Here is a PR with both the changes - #9

@nhandt2021, let us have @meggarr test the changes and then go for merge...
CC: @v1r3n

[meggarr]

@meggarr It also works for me

Can you let me know how to build the docker ? @nhandt2021 .
I am not good at yarn or npm build, I followed the steps in Dockerfile for server and UI, that version just cannot work. The link was //?workflowType=xxx or /search/by-tasks?tasks=yyy in browser's inspection.

[nhandt2021]

@meggarr I followed this instruction: https://github.com/conductor-oss/conductor/tree/main/docker/ui#running-the-conductor-server

Could you provide some screenshots or videos for more detail?

[meggarr]

I followed the steps in here :: https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer . Started the server via docker compose. But, now I have tore down my env.

You can give it a try. @nhandt2021

[haricane8133]

@meggarr, try on top of the PR I've raised.... #9

[nhandt2021]

@meggarr You'r right. I can't run (UI) with that docker compose 😢

[haricane8133]

@meggarr, try on top of the PR I've raised.... #9

Any updates?
@meggarr, @nhandt2021, @v1r3n

Just pinging the way to reproduce the error, in case we went off track with trying docker, etc...

1. Clone the repo
2. Add a temporary NavLink in app.js, passing the newTab param as true (we want the link to open in a new tab), and set the href as /temp
3. yarn build
4. yarn serve-build
5. Click on that NavLink you added.

• The link should fail if you didn't add any HomePage field (default '/' is taken, and because cleanDuplicateSlash() ignores // at the start of the string, we would have the resolved URL as //temp, which would open a new tab with the URL http://temp)
• The link should pass if you set some homepage field.

Now the PR fixes the regex, and the newTab link should work with the default homepage value.

[nhandt2021]

@haricane8133
Have you tried with this case:

I followed the steps in here :: https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer . Started the server via docker compose. But, now I have tore down my env.

[haricane8133]

@nhandt2021,

No I haven't. Why is that needed?
We are trying to test a bug that is related to Navigation within the UI.

You just need to try both

1. DevServer - yarn start
2. ProductionBuild - yarn build + yarn serve-build

(actually considering the bug, one of them should be good enough too)

But the testing should cover all scenarios related to the different ways one can have the homepage field, and different ways one can pass path to the NavLink.

@haricane8133 Have you tried with this case:

I followed the steps in here :: https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer . Started the server via docker compose. But, now I have tore down my env.

I wouldn't consider this as a different case for the testing required for this bug and PR...

If the UI codebase isn't idempotent and has different behaviors on NavLink, with DevServer, ProductionBuild and DockerRun, we are having much bigger problems than this issue :)

[nhandt2021]

@haricane8133 @meggarr
The problem is I still couldn't run that docker compose even I reverted the change 🤔

@haricane8133 Could you run the index.html directly after building? (I can't even revert the change)

[haricane8133]

@haricane8133 Could you run the index.html directly after building? (I can't even revert the change)

This would be the same as yarn serve-build. Yeah. I did that on top of my PR, and it fixes this issue

[meggarr]

Shall we revert changes if it is still not good ?

[haricane8133]

@meggarr, you should let us know...

The PR I have mentioned here, #9, fixes this issue (#7). As I had mentioned before, that PR can be merged and this issue closed....

Test it and update here

I am not from Orkes and don't have merge permissions...

Someone from Orkes can merge this PR

[nhandt2021]

@haricane8133 @meggarr The problem is I still couldn't run that docker compose even I reverted the change 🤔

@haricane8133 Could you run the index.html directly after building? (I can't even revert the change)

I'd tested after reverting. And it didn't work with the docker compose that you mentioned.
@haricane8133 I tested your PR and it couldn't run the index.html directly (inside build folder) same as reverted version

[haricane8133]

@haricane8133 I tested your PR and it couldn't run the index.html directly (inside build folder) same as reverted version

@nhandt2021, I am not able to follow. What do you mean by the PR unable to run the index.html directly? Could you please be more elaborate and specific?

Did you run any command scripts from package.json? You need to run those commands to run the web application.

You need to either build and serve the static files using any server, or use the in built NodeJS Dev Server....

(By the way, to test the UI, you don't have to run the whole conductor repo. Just focus inside the 'ui' folder)

[meggarr]

@haricane8133 I am not from Orkes, so I cannot decide here.
In the meantime, can you pls build Docker Image and try, Docker file is here :: https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer, it doesn't have the "yarn serve-build"

I don't know how to resolve this. Cc @nhandt2021

[nhandt2021]

I don't want to modify the Docker file. I'm finding the the root cause of this

@haricane8133 about your feature, I believe that doesn't relate with @meggarr issue, because he want to run the whole app via docker containers (https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer)
And I reverted the old version, it still didn't work too. That are all my tests.

Btw: Your pr looks good to me. But I prefer using 1 regex instead of using replace 2 times with 2 regular expressions

(By the way, to test the UI, you don't have to run the whole conductor repo. Just focus inside the 'ui' folder)

I think have many people don't care to run UI only. Most of them run the app via Docker.

@haricane8133 I tested your PR and it couldn't run the index.html directly (inside build folder) same as reverted version

I know how to run UI only, and having several ways to do that.
And I can't run the index.html inside build folder in ui folder directly. And one more time I believe it doesn't relate to your PRs.

Look at this, it runs UI as a static page via Nginx. So the problem here is: if we can't run the static page after building so the Docker file either.

[haricane8133]

@meggarr, @nhandt2021, I can certainly try running the Docker file and let you know. I will update the thread.

it doesn't have the "yarn serve-build"

@meggarr, we don't have to use yarn serve-build compulsorily. You can use any static file server like nginx, or simply Python's Simple HTTP Server. It will work.

I know how to run UI only, and having several ways to do that.
And I can't run the index.html inside build folder in ui folder directly.

@nhandt2021, I am still not able to understand what you mean by this. Please be more specific. Did you double click the index.html file?
After you build the UI react application (within /ui), all you have to do is to navigate into the build folder and run any static file server on top. Please make sure that you run the server from the correct directory. If you are having an issue, it would mostly be because the relative paths are not working fine. Either way, we would need details more than "I am unable to run the index.html file" to find out what...

I think have many people don't care to run UI only. Most of them run the app via Docker.

@nhandt2021, it doesn't matter for this issue. If the dockerfile isn't working, let us open another issue and solve it there.
What I am not able to understand is that @meggarr was once able to run the UI application properly to figure out the bug, but is unable to test after the fix is in...?

Btw: Your pr looks good to me. But I prefer using 1 regex instead of using replace 2 times with 2 regular expressions

@nhandt2021, let us continue the discussion on the PR. As I mentioned in the PR, I feel having two .replace() at this point would retain code readability standards. But I am fine to change here. Let me know if you can find a regex that can cover both. I wasn't able to find one.

[meggarr]

@nhandt2021 Is there a direction of this issue ?

[nhandt2021]

@meggarr As I told you before, I reverted and tried with that docker compose file, but it didn't work too. But you said it worked before, so I was confused.
It works perfectly with the current docker files in this OSS repository.

May I have a question: Do you want to use this Basename feature with this docker compose file: https://github.com/orkes-io/orkes-conductor-community/blob/main/docker/DockerfileServer ?

[meggarr]

@nhandt2021 I didn't revert, I use the version of 3.15.0, it works well.

For the 2nd question - No, I don't use the "basename feature", and I don't want that to break the Docker build for UI.

I think we need a decision here to revert this feature totally, or to keep it in future release. Thanks!

Cc @v1r3n @c4lm