Skip to content

Allow notify to run on a different URL#233

Open
jmbrunskill wants to merge 1 commit intomainfrom
232-allow-notify-to-run-on-a-different-path
Open

Allow notify to run on a different URL#233
jmbrunskill wants to merge 1 commit intomainfrom
232-allow-notify-to-run-on-a-different-path

Conversation

@jmbrunskill
Copy link
Copy Markdown
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
@github-actions
Copy link
Copy Markdown

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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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