Skip to content

Enable subdirectory support by default - #3810

Merged
advplyr merged 1 commit into
advplyr:masterfrom
mikiher:enable-subdirectory
Jan 19, 2025
Merged

Enable subdirectory support by default#3810
advplyr merged 1 commit into
advplyr:masterfrom
mikiher:enable-subdirectory

Conversation

@mikiher

@mikiher mikiher commented Jan 9, 2025

Copy link
Copy Markdown
Contributor

Brief summary

Switch to enabling subdirectory support with /audiobookshelf on the server and web client, according to the plan laid out in discussion #3535.

Which issue is fixed?

This finally fixes #385

Users will now be able to set their reverse proxies to proxy /audiobookshelf subdirectory requests

In-depth Description

This is the last step in supporting subdirectory with the existing static site generation approach.

The plan is described in detail in #3535.

As a reminder, this will set a fixed subdirectory of /audiobookshelf on the web client (non-subdirectory paths will get rewritten by the server), and the server will support both subdirectory requests (with /audiobookshelf prefix) and non-subdirectory requests (with no prefix).

Note: Mobile app support for subdirectory is already implemented and checked in with audiobookshelf-app PR #1417, but is pending release of a new version. Other mobile clients that would like to support subdirectory access will likely need to make some changes in the spirit of that PR.

How have you tested this?

This PR just switches the feature on by default.

The many changes required to make this happen were previously tested in respective previous PRs, and were tested again to make sure they still worked now.

One thing I haven't tested properly is the reverse proxy part, since my reverse proxy (on my Synology NAS) doesn't support support subdirectory proxying. I am sure this works well once you set it up on a supporting reverse proxy (like NGINX), but haven't tested this myself.

@mikiher
mikiher marked this pull request as ready for review January 9, 2025 07:27
@advplyr

advplyr commented Jan 19, 2025

Copy link
Copy Markdown
Owner

Thanks!

@advplyr
advplyr merged commit 58ca264 into advplyr:master Jan 19, 2025
@4ch1m

4ch1m commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

The new release (Docker container 2.18.0) - which includes/activates this feature - works perfectly fine for me in both the browser and the Android app.

I'm super happy that @mikiher made the effort to implement it. 👍
Again... thank you very much. 🙇

However, I think there are few things that should be revised:

  • The environment variable ROUTER_BASE_PATH is (in it's current state) a bit confusing, since it doesn't seem to have any effect at all; or can't actually be used to change the path value.
    (I'm aware that @mikiher explained that the /audiobookshelf path is hardcoded now due to several technical reasons.)
    So why exactly does it still exist?

  • The README still says:

    Note: Subfolder paths (e.g. /audiobooks) are not supported yet. See https://github.com/advplyr/audiobookshelf/issues/385`

    This could/should be removed now, I guess.

  • The README section containing all reverse proxy setup examples needs to be updated.
    I'm using Apache2 and got everything working with this adjustment:

RewriteRule (.*)/audiobookshelf/?(.*) "ws://127.0.0.1:<audiobookshelf_port>/audiobookshelf/$2" [P,L]

(Not sure if there's a simpler solution though.)

@mikiher

mikiher commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

The new release (Docker container 2.18.0) - which includes/activates this feature - works perfectly fine for me in both the browser and the Android app.

I'm super happy that @mikiher made the effort to implement it. 👍 Again... thank you very much. 🙇

However, I think there are few things that should be revised:

  • The environment variable ROUTER_BASE_PATH is (in it's current state) a bit confusing, since it doesn't seem to have any effect at all; or can't actually be used to change the path value.
    (I'm aware that @mikiher explained that the /audiobookshelf path is hardcoded now due to several technical reasons.)
    So why exactly does it still exist?

It is still possible to use the environment variable to change the /audiobookshelf path, but only if you build the code yourself (i.e. set the environment variable then run npm run client). This isn't something most of our users do, hence my use of term "hardcoded".

You're right. We'll remove this.

  • The README section containing all reverse proxy setup examples needs to be updated.
    I'm using Apache2 and got everything working with this adjustment:
RewriteRule (.*)/audiobookshelf/?(.*) "ws://127.0.0.1:<audiobookshelf_port>/audiobookshelf/$2" [P,L]

(Not sure if there's a simpler solution though.)

Great! We should definitely provide at least one or two examples of modifying reverse proxy setups for subdirectory support (the existing setups should still work for subdomain). I'm not sure we can be responsible for providing details for every reverse proxy out there, but we definitely should encourage users to share their setups with us and publish them.

You're also welcome to submit a PR to change the documentation, if you like!

@beschoenen

Copy link
Copy Markdown

Shouldn't the default be 'audiobookshelf'? I'm trying to host this application on /:user/audiobookshelf and I'm having a hard time since the Docker is precompiled with /audiobookshelf. Removing the slash would make it relative to the current subfolder in the url.

Comment thread client/nuxt.config.js
const pkg = require('./package.json')

const routerBasePath = process.env.ROUTER_BASE_PATH || ''
const routerBasePath = process.env.ROUTER_BASE_PATH || '/audiobookshelf'

@devnoname120 devnoname120 Feb 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mikiher When the ROUTER_BASE_PATH env variable is set to an empty string in order to restore the old behavior then it overrides it to /audiobookshelf.

Is that expected? I would have thought that the /audiobookshelf fallback should only have been used in the case where the ROUTER_BASE_PATH env variable is undefined.

Suggested change
const routerBasePath = process.env.ROUTER_BASE_PATH || '/audiobookshelf'
const routerBasePath = process.env.ROUTER_BASE_PATH ?? '/audiobookshelf'

Note that the nullish coalescing operator ?? was added in Node.js 14 and we currently require Node.js ≥ 20 so it shouldn't be a problem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

devnoname120 added a commit to devnoname120/audiobookshelf that referenced this pull request Feb 10, 2025
When the `ROUTER_BASE_PATH` env variable is set to an empty string it's mistakenly overriden to `/audiobookshelf` instead.
The `/audiobookshelf` fallback should only be used when the `ROUTER_BASE_PATH` env variable is undefined, not just an empty string.

Regression introduced in advplyr#3810
See also: advplyr#3810 (comment)

Partially address advplyr#3874
@mikiher
mikiher deleted the enable-subdirectory branch July 24, 2025 10:45
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.

[Enhancement]: Add subfolder URL path instead of using a subdomain

5 participants