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

Allow notify to run on a different URL #233

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmbrunskill
Copy link
Collaborator

Closes #232

πŸ‘©πŸ»β€πŸ’» What does this PR do?

This makes it possible to run notify on a path like https://yourserver.yourdomain.com/notify/

πŸ§ͺ How has/should this change been tested?

I've tried this with Caddy using a Caddy file like this.

:7007 {
	respond 404
	redir /notify /notify/
	handle_path /notify/* {
		reverse_proxy http://localhost:8007
	}
}

To serve the frontend via the backend server, you also need to run yarn build.

πŸ’Œ Any notes for the reviewer?

Anything else I might have missed?

@jmbrunskill jmbrunskill linked an issue Nov 8, 2023 that may be closed by this pull request
Copy link

github-actions bot commented Nov 8, 2023

Bundle size difference

Comparing this PR to main

Old size New size Diff
2.11 MB 2.11 MB 499 B (0.02%)

@mark-prins mark-prins self-assigned this Nov 16, 2023
Copy link
Collaborator

@lache-melvin lache-melvin left a comment

Choose a reason for hiding this comment

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

Found this... I can navigate to any page, but refreshing only works when I'm not on a nested route? I.e. I can refresh fine on /recipients, but trying to refresh on /recipients/sql, I get a blank page and these errors:

Screenshot 2023-11-22 at 9 33 44 AM

@lache-melvin
Copy link
Collaborator

After some investigation into this, it looks like we'd need to choose between making the app available on a dynamic path (publicPath: auto), or using nested paths within the app. When loading the app on a nested route, without a predefined publicPath, Webpack's only option is to assume the app is being loaded on what it should consider the base path, so everything breaks.

Assuming we'd like to continue not constraining our app to a single level of routes, we could hardcode the publicPath to something other than /, as proposed in the issue, e.g. if /notify would better suit. Or we could inject the path as an environment variable.

Be good to catch up on this/jam on what the best way forward would be @jmbrunskill πŸ™

@lache-melvin
Copy link
Collaborator

Update on this was that it is probably best to leave, sadly, couldn't find a solution that came without falling over somewhere else...

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.

Allow notify to run on a different path
3 participants