Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Make public path configurable via an environment variable #203

Merged
merged 5 commits into from Jan 19, 2021
Merged

Make public path configurable via an environment variable #203

merged 5 commits into from Jan 19, 2021

Conversation

jeromefroe
Copy link
Contributor

@jeromefroe jeromefroe commented Nov 18, 2020

Hi Temporal team! This PR addresses #179 by making the root path that the app is served under configurable via the TEMPORAL_WEB_ROOT_PATH environment variable. This approach is based on the webpack documentation for the publicPath configuration option. Thanks!

upd:
image

@swyxio
Copy link
Contributor

swyxio commented Nov 18, 2020

@feedmeapples should be a straightforward one hopefully - can you test and merge if it looks good?

@jeromefroe for your usecase, have you verified that our /api/ and /auth/ routes dont need to be namespaced as well?

@jeromefroe
Copy link
Contributor Author

@sw-yx this PR is going to need more work. I've done more testing and it looks like the Vue client needs to be updated as well to use the appropriate root path. I've tried updating the base configuration option of the Vue Router but it looks like more changes are still needed.

@swyxio swyxio marked this pull request as draft November 19, 2020 02:33
@swyxio
Copy link
Contributor

swyxio commented Nov 30, 2020

just checking in - hows it going?

@jeromefroe
Copy link
Contributor Author

just checking in - hows it going?

Hey @sw-yx! I think this PR is getting close, I've updated both the server-side and client-side to support being deployed under a subpath, but I've noticed that links will occasionally be redirected to URL's which duplicate the subpath. For example, if the subpath is /temporal then although the link will be to /temporal/api/foo it will get redirected to /temporal/temporal/api/foo which will result in a 404. Any pointers would be much appreciated since I'm not very familiar with Vue :)

@swyxio
Copy link
Contributor

swyxio commented Dec 1, 2020

this might be a base Url issue. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base

@feedmeapples any idea?

@feedmeapples
Copy link
Contributor

feedmeapples commented Dec 8, 2020

looking into it to help and make it work. sry for the delay

@nathancastelein
Copy link

any news on this pull request ? definitely waiting for it to have a proper temporal dashboard on my prod environment. Thanks !

@swyxio
Copy link
Contributor

swyxio commented Jan 15, 2021

no updates yet - i dont mind this in principle, just want to make sure @feedmeapples is fine with the code as he's responsible for maintaining going fwd

@feedmeapples
Copy link
Contributor

feedmeapples commented Jan 15, 2021

no updates yet - i dont mind this in principle, just want to make sure @feedmeapples is fine with the code as he's responsible for maintaining going fwd

i need to do something with the links, in the current state they end up duplicating the subpath and breaking the app. Looking into this again rn

The links are generated using vue-router's router-link. Checking the documentation for this

@feedmeapples feedmeapples marked this pull request as ready for review January 16, 2021 06:04
@feedmeapples
Copy link
Contributor

feedmeapples commented Jan 16, 2021

Found what was causing subpath duplication

There was a code in App.vue -> globalClick that was intercepting link clicks. This code was messing the vue-router basePath behavior and adding additional subpath everytime links were clicked. Based on the code comment, this code was needed for mocha tests only. Since we switched to cypress, it shouldn't be needed anymore

Removing this code also fixes ctrl+click not opening links in a new tab

@sw-yx @rylandg

@feedmeapples feedmeapples linked an issue Jan 16, 2021 that may be closed by this pull request
@feedmeapples
Copy link
Contributor

This should be good to check in with one more approve 👍

@feedmeapples feedmeapples self-assigned this Jan 16, 2021
@feedmeapples feedmeapples merged commit 743ff25 into temporalio:master Jan 19, 2021
@feedmeapples
Copy link
Contributor

@jeromefroe this feature seems had an issue in production build. I've merged changes to address this #339

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants