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

Enable hosting from subdirectory #771

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Pascal-So
Copy link

As a server admin I want to host organice from a subdirectory URL in order to arrange my server's routes to my liking and to have more technical flexibility (i.e. host organice from the same domain as nextcloud so that I don't have to set up CORS).

This fixes issue #732.

This implementation uses CRA's homepage feature in package.json. The person wanting to host organice from a subdomain should then adjust the homepage value to the full desired domain and path, for example "homepage": "https://example.com/organice/".

After running yarn build as usual, they can then upload the resulting build output to that subdirectory on their server. Before this change, this was not possible because the router would have kept redirecting to the "/" route, the assets were not found, etc.

Internally, CRA takes the homepage value and extracts the path part of it, which is then assigned to PUBLIC_URL. It would also be possible to build organice without adjusting the homepage value and instead just calling PUBLIC_URL=/organice yarn build.

In my PR I left the homepage value as "" which should be safe as that should lead to the same behaviour as not specifying it. See the relevant implementation in CRA here (the code doesn't check for undefined, it just casts to bool).

Do you want me to add some documentation for this? If yes, where? Should I add a new section in the FAQ for this?

@Pascal-So
Copy link
Author

hmm, circleci fails with error 'process' is not defined...

But I noticed that the code is already using the process object in SyncServiceSignIn, so does anyone know why this fails for my code?

@Pascal-So
Copy link
Author

Ah I see, we need to tell eslint that the process global exists: https://eslint.org/docs/user-guide/configuring/language-options#specifying-globals

Instead of adding the /* global process */ comment everywhere, would it make more sense to add that to the .eslintrc.yml file instead?

@@ -35,7 +35,7 @@ function WebDAVForm() {
persistField('webdavEndpoint', url);
persistField('webdavUsername', username);
persistField('webdavPassword', password);
window.location = window.location.origin + '/';
window.location = window.location.origin + process.env.PUBLIC_URL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it behave if PUBLIC_URL is undefined instead of /?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good catch, thanks. It just turns into "" so the trailing slash would be missing. It looks like trailing slashes are removed from the PUBLIC_URL anyway (for both the homepage method and directly specifying PUBLIC_URL) so I'll simply re-add the + '/' after the PUBLIC_URL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, but does it also work for export PUBLIC_URL='/' (which should be allowed IMO)? Sorry for that annoyance; I also don't have a solution by hand right now...

Copy link
Author

Choose a reason for hiding this comment

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

Yes I also tested that, this behaves the same as the other cases i.e. the slash is removed and PUBLIC_URL ends up as "" which is what we want.

@Pascal-So
Copy link
Author

Pascal-So commented Jan 12, 2022

I was also wondering what would happen for a subdirectory that contains the string 'files', see this snippet in renderFileBrowserBackButton():

const fileParts = window.location.href.split('/').map((e) => decodeURIComponent(e));
if (_.includes(fileParts, 'files')) {
  backPath = _.last(fileParts);
}

Hosting this at https://example.com/organice/files now leads to the problem that the settings page renders like this:

image

instead of like this:

image

There are however two remarks I have about this situation:

  1. It doesn't seem very likely that someone would use files as one of the directory names in the path to their organice instance, worst case we can warn against it in the documentation.
  2. The back button text appears to be misappropriated for the directory name when browsing files. In my opinion the back text should always be "Back", and the current directory name should be printed somewhere else:
    a. either centered in the title bar just like the "Settings" text on the settings page
    b. or if that leads to problems with the directory name being too long then maybe add a breadcrumb line below the title bar. That way the whole path would be visible, not just the name of the current directory.

With the directory name being displayed somewhere else, we could then change the renderFileBrowserBackButton() to always display the "Back" text instead of something dynamic. Also, clicking the directory name wouldn't take you out of the directory whose name you just clicked anymore.

image

Therefore I don't think this issue is significant.

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.

None yet

2 participants