Prefix path router

[giuliaghisini]

Enable to publish Volto site under a prefixPath, for example www.mymainsite.com/prefixPath

[netlify[bot]]

✅ Deploy Preview for volto ready!

Name	Link
🔨 Latest commit	1e2620e826c1df63d0a31fc3db76c0bc4ecf5d6a
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/65f8677e35abd400086369d8
😎 Deploy Preview	https://deploy-preview-3592--volto.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[cypress[bot]]

Passing run #7234 ↗︎

0

14

0

0

0

Details:

refactor: prettify components
Project: Volto	Commit: ef6216a98a
Status: Passed	Duration: 02:07 💡
Started: Nov 28, 2023 11:05 AM	Ended: Nov 28, 2023 11:07 AM

Review all test suite changes for PR #3592 ↗︎

[pnicolli]

Link view now working, Image view still has issues with the download link which is not working (see this example)

[sneridagh]

During the Volto Team meeting we agreed that we would have to put in place a whole round of Cypress tests pointing to a deployment using this. I can imagine that in the future will be quite easy to break the whole feature if one does not have in mind it.

I think that could be time consuming, but might not have much difficulty. What do you think?

[sneridagh]

@mamico @giuliaghisini could you share the reverse proxy config you have on such deployments?

[sneridagh]

We should add some documentation about how to setup such a deployment. Also, I'll try to write a traefik config for testing it properly in CI.

[cekk]

@sneridagh our deployment config is a bit complicated because there are several urls and frontend names, and it's made in varnish and not in nginx/apache.

The conf itself for using this branch is easy. Here is an example for nginx:

upstream backend {
  server host.docker.internal:8080;
}
upstream frontend {
  server host.docker.internal:3000;
}

server {
  listen 80  default_server;
  server_name  localhost;

  location ~ /foo/\+\+api\+\+($|/) {
      rewrite ^/foo/\+\+api\+\+($|/.*) /VirtualHostBase/http/${server_name}/Plone/++api++/VirtualHostRoot/_vh_foo$1 break;
      proxy_pass http://backend;
  }

  location ~ /foo($|/) {

    # the standard nginx conf
    ...

    proxy_pass http://frontend;
  }
}

[tiberiuichim]

@cekk One other thing is important to document, the prefix path /foo corresponds to the Plone root, not a /foo subfolder in Plone.

[cekk]

Yes, /foo points to the root of Plone site

[sneridagh]

@cekk To make it work I had to launch Volto with:

RAZZLE_PREFIX_PATH=/foo RAZZLE_API_PATH=http://localhost/foo yarn start

I would have expect that given a RAZZLE_PREFIX_PATH, the other would have adjusted automatically (as seamless mode promises). I am doing something wrong? because given a look at the code, it seems it should, right?

[sneridagh]

@cekk Forget the question, I'm still asleep. 😅

[sneridagh]

Added tentative tests: #3719 see comments.

[nileshgulia1]

I would like to take this forward. Locally looks good. I will checkout this branch on one of our projects first and see if I find some issues.

State of this PR:

This PR is based on basename react-router prop which adds an initial entry to browser "history" stack and allows all links to be prefixed with basename by react-router. The static assets like images need to be explicitly prefixed. I amended an express-middleware which re-directs to app base path on initial load.

There is another approach in this PR, which uses a store enhancer middleware to prefix all the router paths(amending history accordingly) and modified flattenToAppUrl method.

What's left are the cypress tests https://github.com/plone/volto/pull/3719/ which should also account for prefix path in the URLs. I will try to have a look into them.

I personally like the basename approach and let react-router handle the prefixes. However, we need to think about the non-router links and static assets. What do you think @sneridagh @davisagli @tiberiuichim @pnicolli ?

[wesleybl]

@giuliaghisini @nileshgulia1 @sneridagh any chance we have this feature in master?

[wesleybl]

I updated the branch and run:

RAZZLE_PREFIX_PATH=/my-prefix yarn start

Then I got the error:

ValidationError: Invalid options object. Dev Server has been initialized using an options object that does not match the API schema.
 - options has an unknown property 'publicPath'. These properties are valid:
   object { allowedHosts?, bonjour?, client?, compress?, devMiddleware?, headers?, historyApiFallback?, host?, hot?, http2?, https?, ipc?, liveReload?, magicHtml?, onAfterSetupMiddleware?, onBeforeSetupMiddleware?, onListening?, open?, port?, proxy?, server?, setupExitSignals?, setupMiddlewares?, static?, watchFiles?, webSocketServer? }
    at validate (/home/user/git/volto/node_modules/webpack-dev-server/node_modules/schema-utils/dist/validate.js:115:11)
    at new Server (/home/user/git/volto/node_modules/webpack-dev-server/lib/Server.js:231:5)
    at new razzleDevServer (/home/user/git/volto/node_modules/razzle/config/razzleDevServer.js:10:5)
    at /home/user/git/volto/node_modules/razzle/scripts/start.js:181:33
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

I'll take a look at it.

[tiberiuichim]

@wesleybl it's due to the changes in razzle.config.js

[wesleybl]

@tiberiuichim I fixed this in: https://github.com/plone/volto/pull/3592/commits/f011df6ea0a6e2c86dbb58a5abbb143887d17357

[wesleybl]

Documentation and testing was done in #3719 and merged here.

[wesleybl]

I did the rebase with the master and did tests. Everything looks ok now. Can you please have a look and merge?

@sneridagh @nileshgulia1 @pnicolli @cekk @tiberiuichim

[nileshgulia1]

Let's discuss this in today's @plone/volto-team meeting!

[nileshgulia1]

Note: There's still an issue with asyncConnect Calls. I'm looking into it.

[ericof]

@sneridagh, @nileshgulia1, what is still needed to merge this to 18?

[nileshgulia1]

Note: There's still an issue with asyncConnect Calls. I'm looking into it.

Fixed in 3ad97df. @ericof I'm ready on my part. It would be great if we have more eyes on it. Thanks @wesleybl for review!

[ericof]

@sneridagh, @davisagli Could you take a look at this as well?

[netlify[bot]]

✅ Deploy Preview for plone-components canceled.

Name	Link
🔨 Latest commit	1e2620e826c1df63d0a31fc3db76c0bc4ecf5d6a
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/65f8677eb7efa500089b56b6

[teekuningas]

Hello! Just tested with newest volto and seems to still work fine. However, it seems that the cookies are still set with path "/"? When multiple volto's are hosted under same domain, like https://cat.nip/site1 and https://cat.nip/site2, the cookies seem to be shared and then for example if logged in to one site, the other site will show "Unauthorized" everywhere. What would be the best fix?

[stevepiercy]

@teekuningas this is a pull request that has not been released. Did you apply this pull request in your project to test it out?

If not, then you can search the issue tracker for your issue https://github.com/plone/volto/issues?q=is%3Aissue+is%3Aopen+cookie. If you need support, then post to https://community.plone.org/.

[teekuningas]

Yes, by creating and applying patch. Otherwise it seems to work as expected but the multiple sites under same domain is a problem. What we want is to lightly create new sites using a single docker image and give each a different prefix.

[teekuningas]

This might be enough:

diff --git a/packages/volto/src/helpers/Cookies/cookies.js b/packages/volto/src/helpers/Cookies/cookies.js                                                                                                        
index a51d2b1aa..e2ea0f6e5 100644
--- a/packages/volto/src/helpers/Cookies/cookies.js
+++ b/packages/volto/src/helpers/Cookies/cookies.js
@@ -1,7 +1,7 @@
 import config from '@plone/volto/registry';

 export const getCookieOptions = (options = {}) => {
-  const { path = '/', secure, ...otherOptions } = options;
+  const { path = config.settings?.prefixPath || '/', secure, ...otherOptions } = options;
   let secureOption = secure;

   try {

[teekuningas]

Also, there are some changes to Image.jsx from 2 months ago that should be taken into account.. this might be enough:

diff --git a/packages/volto/src/components/theme/Image/Image.jsx b/packages/volto/src/components/theme/Image/Image.jsx
index db4125554..39ae2013c 100644
--- a/packages/volto/src/components/theme/Image/Image.jsx
+++ b/packages/volto/src/components/theme/Image/Image.jsx
@@ -67,7 +67,7 @@ export default function Image({
       attrs.srcSet = sortedScales
         .map(
           (scale) =>
-            `${flattenToAppURL(basePath)}/${scale.download} ${scale.width}w`,
+            `${addPrefixPath(flattenToAppURL(basePath))}/${scale.download} ${scale.width}w`,
         )
         .join(', ');
     }

[wesleybl]

addPrefixPath must also be used in the Logo component

Done in https://github.com/plone/volto/pull/3592/commits/61d96d2a48d9cdfc11b841f3050700b5d0facec4

[wesleybl]

Also, there are some changes to Image.jsx from 2 months ago that should be taken into account

@teekuningas done in https://github.com/plone/volto/pull/3592/commits/c7938167f5f28334f37eca55f8b132f70c382b2a

[wesleybl]

I updated the branch, fixed some issues and tested. It's all OK. Can anyone else review? Can we merge this PR?

@sneridagh @tiberiuichim @nileshgulia1 @pnicolli @ericof

[nileshgulia1]

@wesleybl I was wondering why those 26 tests skipped. As far as I remember we should test this also with multilingual websites and in seamless mode covering all core cypress tests.

[wesleybl]

I was wondering why those 26 tests skipped.

@nileshgulia1 these tests that are skipped are only executed when there is a pull request made from a fork. This occurs in any pull request. See:

https://github.com/plone/volto/blob/ffef3f0f0615f6fd7a6298526dee4f760c59818d/.github/workflows/acceptance.yml#L5

As far as I remember we should test this also with multilingual websites and in seamless.

Multilingual tests are already executed with prefix path:

https://github.com/plone/volto/blob/48554dfcdfd31789c13e9b19ad4006a03fee10b7/.github/workflows/acceptance.yml#L1315

seamless not yet. I'll add him.

[wesleybl]

As far as I remember we should test this also with multilingual websites and in seamless mode covering all core cypress tests

@nileshgulia1 we have a lot of cypress tests, as can be seen in: https://github.com/plone/volto/tree/main/packages/volto/cypress/tests

Should all these tests be run with prefixed? I'm afraid the tests will take too long.

[wesleybl]

@nileshgulia1 hmm, I noticed now that you suggested core tests. Minimal tests are being tested today. I will replace it with core tests.

[wesleybl]

@nileshgulia1 @sneridagh some tests expect a URL without the prefix. For example:

https://github.com/plone/volto/blob/81d0369e1628b3072f9e47d14831a2618d2633b6/packages/volto/cypress/tests/core/basic/actions.js#L65

However, when we run the prefixed tests, we have the prefix there and the test fails.

What is the best option in this case? Do a test using the RAZZLE_PREFIX_PATH environment variable? Test only the end of the URL?

[sneridagh]

@wesleybl I'd add the prefix path, if present, in a way that it's not much intrusive and readable, if possible. You can get it from the window object, or from the Volto config (there's a helper for it).

[wesleybl]

@sneridagh I managed to do it with an environment variable. For example:

describe('Tests', () => {
  let prefixPath;
  beforeEach(() => {
    prefixPath = Cypress.env('prefixPath') || '';
  });
  it('test', function () {
    const url = `${prefixPath}/my-page-rename/contents`
  });
});

[nileshgulia1]

@nileshgulia1 @sneridagh some tests expect a URL without the prefix. For example:

https://github.com/plone/volto/blob/81d0369e1628b3072f9e47d14831a2618d2633b6/packages/volto/cypress/tests/core/basic/actions.js#L65

However, when we run the prefixed tests, we have the prefix there and the test fails.

What is the best option in this case? Do a test using the RAZZLE_PREFIX_PATH environment variable? Test only the end of the URL?

We need to strip out the prefix paths for some cases like copy/renaming contents or creating aliases. You could write an exception if there is a prefix, then remove it 🤔

[nileshgulia1]

@wesleybl Did you checked if the moving/renaming of content works locally in this branch? We are using another solution in https://github.com/eea/volto-prefixpath/tree/master which might not be the case with other prefixed sites.

[wesleybl]

We need to strip out the prefix paths for some cases like copy/renaming contents or creating aliases. You could write an exception if there is a prefix, then remove it

@nileshgulia1 I will try to use this: https://github.com/plone/volto/pull/3592#issuecomment-1997827066

Did you checked if the moving/renaming of content works locally in this branch?

Yes, this works.

[wesleybl]

@nileshgulia1 I needed to revert part of the commit: https://github.com/plone/volto/commit/3ad97df8467f54d72a1fa1a5c85ce093bef7e3d3

The problem of not loading properties occurs in more routes, not just in /controlpanel. We need a more generic solution. See: https://github.com/plone/volto/pull/3592/commits/9a6b25f08b11f463b7d15b3af3a2151aa6b99407

I will check this.

[wesleybl]

@nileshgulia1 I fixed the problem by adding prefixed routes. See: https://github.com/plone/volto/pull/3592/commits/149c8998babe318387722f9b7d26240c1c8615e1

Please check if it works for you.

[wesleybl]

@sneridagh @nileshgulia1 add core test with prefixed path. Can you take a look please? Can we merge this PR?

[wesleybl]

This is intrusive enough for it to have its own major release alone, and I won't include it in 18...

@sneridagh so would this be for Volto 19? Any predictions on when Volto 19 will start?

[stevepiercy]

Any predictions on when Volto 19 will start?

It already has, by virtue of this pull request. I think this is the second feature designated for Volto 19, after my PLIP for timezone, date, and time support. I think it would be good to create a PLIP for discussion and add it to the Volto Roadmap GitHub project. I also created a 19.x.x milestone to start tagging issues and pull requests.

[nileshgulia1]

Minutes from the @plone/volto-team meeting:

1. It was suggested that in dev mode, the prefix should not be a concern. Instead, configurations should be adjusted to exclusively utilize prefix during deployment.
2. Explore the possibility to integrate the functionalities of addprefixPath into flattenToAppUrl covering all the edge cases we might have. react-router v6 has basename support. We need to further investigate how this feature is intended to function, both with and without the React Router upgrade( we are still using v5). The current implementation may compel future developers to rely extensively on this additional helper. Also what about the existing addons? they will have to be migrated as well.
3. The current cypress testing strategy needs a rehaul according to point 2. It looks unsustainable to incorporate addPrefixPath across all forthcoming tests. Perhaps there is a way to run tests just using a cypress baseUrl? What about the internalUrls those need to be prefixed like Images, links?
4. In the context of deployment documentation, we should explicitly outline if we need to use apache rules or docker.