Prefix path router

[giuliaghisini]

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

[netlify]

❌ Deploy Preview for volto failed. Why did it fail? →

Name	Link
🔨 Latest commit	a38dda0
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/6645ebd8e199fb0008479210

[cypress]

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 #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: f011df6

[wesleybl]

@nileshgulia1 @sneridagh Cannot add prefix in flattenToAppUrl. To see:

#3592 (comment)

So I returned with the addPrefixPath function, and called it where necessary. I don't see a problem with using this function this way. Basically it is called in structural components, with UniversalLink and Image. From what I understand, the use of these components is strongly recommended. So "forgetting to prefix" will be very rare. If we use these components, the prefix will already be done.

I have the time and sponsorship to work with this functionality now. From my point of view, it's working well. Tests are passing now. I need guidance on what I should do to improve this PR.

[wesleybl]

Why do you need to restart the server? What you mentioned here probably because of broken devServer. I will look into it tomorrow.

@nileshgulia1 so can I go back to reading the RAZZLE_PREFIX_PATH variable in development mode?

[nileshgulia1]

@wesleybl so the idea is like this, if we remove prefix-path addition in flattenToAppUrl, and since we are using the basename we don't have to prefix manually. All the links with a react-router <Link/> should already have a prefix. The basename=/prefix means by-default / points to /prefix. So if you have anywhere in your project, <Logo href="/" /> you will end up with a trailing slash like so /prefix/. This is a bug in react-router v5. They fixed that in latest version, dunno know if it was backported.

Regarding the development recipe, I think the website itself shouldn't care about the prefix in dev. The prefix should only be used in production. If you want to test it, you can do RAZZLE_PREFIX_PATH=/prefix yarn build && RAZZLE_PREFIX_PATH=/prefix yarn start:prod, and see if it has the prefix.

[nileshgulia1]

So I returned with the addPrefixPath function, and called it where necessary. I don't see a problem with using this function this way. Basically it is called in structural components, with UniversalLink and Image

I think it's worth a try not to depend on addPrefixPath function. I would not force anyone to use it in their new addons or components.

[wesleybl]

If you want to test it, you can do RAZZLE_PREFIX_PATH=/prefix yarn build && RAZZLE_PREFIX_PATH=/prefix yarn start:prod, and see if it has the prefix.

@nileshgulia1 the problem is precisely when we are developing the prefix functionality and we want to see the website with the prefix. The RAZZLE_PREFIX_PATH=/prefix yarn build command takes a long time. And for every change we make to a file, we need to compile it again to see the changes. Hot reload does not work in production mode. What is the advantage of not using RAZZLE_PREFIX_PATH in development mode? I think we should allow this, so that it is not necessary to compile for each modification.

[nileshgulia1]

the problem is precisely when we are developing the prefix functionality and we want to see the website with the prefix.

That is the thing, why do we want to develop with prefix differently from a normal case? If you want to develop with prefix, you shouldn't do anything special in development. At least that's what we have discussed in @plone/volto-team meeting. Can't we keep things minimal? So just do RAZZLE_PREFIX_PATH=/preffix yarn build && RAZZLE_PREFIX_PATH=/preffix arn start:prod. In any case I would like to hear more thoughts. -cc @sneridagh @giuliaghisini @cekk

[wesleybl]

That is the thing, why do we want to develop with prefix differently from a normal case?

@nileshgulia1 when I say "developing the prefix functionality", I'm talking about situations like fixing the test of a prefixed CI job. Or making changes like prefixing inside or outside the flattenToAppUrl function. In these cases, I want to use the RAZZLE_PREFIX_PATH variable in development mode, so that it is not necessary to compile with each file change.

For "normal development", we can start without the variable, as is normally done today. It would be transparent.

[wesleybl]

I think it's worth a try not to depend on addPrefixPath function. I would not force anyone to use it in their new addons or components.

@nileshgulia1 In order to avoid having an addPrefixPath function and prefixing in the flattenToAppUrl function, we need to remove the basename from the route, so as not to duplicate the prefix in the Link and UniversalLinks components, since the flattenToAppUrl function is called in these components. But everywhere I've researched about how to use prefixes in React, it's recommended to use basename in the route. So I'm not sure if it would work without the basename. This could make things more complicated than having the addPrefixPath function. Even if it works without the basename and the prefix is ​​done in flattenToAppUrl, I think we would still have situations in which the action performed by flattenToAppUrl would be necessary but would not require prefixing. It won't always be necessary to do both.

I'll try to make a branch removing basename to see how things behave.

[wesleybl]

@nileshgulia1 I'm sorry but I'm going to enable reading of the RAZZLE_PREFIX_PATH environment variable in development mode. I'm trying to implement the removal of basename from the route and it's really annoying having to keep compiling the files every time a file changes. Maybe I didn't understand why not reading this variable in development mode. If you still think that not reading the variable is a good thing, we can discuss this, and come back if necessary. But I don't see how this helps.

[wesleybl]

@nileshgulia1 @sneridagh I'm implementing the removal of basename from the route and the creation of the prefix in the flattenToAppURL function. It works. But we would have to use the flattenToAppURL function everywhere that uses the Link component. I noticed that in many places where the Link component is used, the flattenToAppURL function is not used. For example:

volto/packages/volto/src/components/manage/Controlpanels/Controlpanels.jsx

Line 209 in 6f18663

<Link to={`/controlpanel/plone-upgrade`}>

volto/packages/volto/src/components/manage/Controlpanels/Controlpanels.jsx

Line 231 in 6f18663

<Link to={`/controlpanel/${controlpanel.id}`}>

In the end, we would have to change many more files than with the use of addPrefixPath separately. So in my opinion, it is better to keep the addPrefixPath function and the use of basename in the route. What do you think?

[sneridagh]

@wesleybl sorry, I can't follow. Tomorrow I can try to take a full read, but at this time of the year I can't devote any effort to make this happen. What do you mean by removing the basename from the router? This is the full base of the feature and we can't remove it. So I'm quite shocked.

We've talked about this feature during today's Volto team meeting.

1. This feature is NOT trivial.
2. It should be done in a conscious, sustainable way, with enough vision and not jeopardizing the future development of Volto.
3. We can't just hack the full Volto routes and branch the whole code only because of that.
4. If we can't do it under these premises, then it won't happen. We have to find the method to do it in the right, elegant way. Not quick and dirty. TBH, I'd love that someone else from the Volto Team steps up and help with this.
5. The base set by RedTurtle in the initial PRs was good but somehow it derived and derailed in some bad decisions.
6. Right now I can't put any energy on this, probably I won't until some sprint happens, and it seems that only @nileshgulia1, EdW and you are interested in that, if so, we will need resources when it matters (see below).
7. We need consensus from the Volto Team, let's focus on revisit the existing PLIP (Prefix path setup #4290), add there all the findings and problems, and discuss how to solve it. We need to update the premisses and requirements too.
8. We need someone who really needs it and have it working and have the feedback from it. This can't happen if it's not battle tested.

So I propose to start over in a new PR during a sprint, in a joint effort, and go slowly, step by step, argumenting every change with comments, if needed. There has been so many people working in this PR, that I bet that there are changes in there that nobody knows why they are there. Ferrara happens at the end of September. We can start set the bases in there. The Plone Conf is not that far, and the ones that are interested in the feature will attend there.

[sneridagh]

@wesleybl ok, @nileshgulia1 created the PLIP: #4290

I'd love to talk to you as well in person (I already did with Nilesh this morning in the Volto Team Meeting). I'm open for a talk during this week (not on Thursday) Let me know if you can.

[nileshgulia1]

@wesleybl I agree that we should have a basename set, that is the starting point in the PR, it takes care of prefixing for us. We need to identify the parts where the prefix is manually needed. For me, focus is more on trying to avoid affecting the future development of volto. Or if we should really introduce env vars starting with RAZZLE_ now. There are lof of efforts that uses another build system and maybe in future we might shift there.
@wesleybl If you plan to attend PloneConf, then let's discuss this in detail there during sprints or in general.

[wesleybl]

We need to identify the parts where the prefix is manually needed.

@nileshgulia1 basically, we need to manually prefix in the HTML tags a and img, since React Router does not influence these tags. That's why we need the addPrefixPath function. In this PR, we have already replaced the use of the img tag with the Image component, and we use addPrefixPath inside the Image component. Volto also practically does not use the a tag for links. It doesn't even make sense to use the a tag in React. The only situation in which the a tag is necessary is for file downloads. But file downloads are encapsulated in the UniversalLink component and there we use addPrefixPath.

In short, if we use the UniversalLink, Link and Image components, everything will be fine.

[wesleybl]

I'd love to talk to you as well in person

@sneridagh I sent you an email.

[nileshgulia1]

I'd love to talk to you as well in person

@sneridagh I sent you an email.

I'll be happy to join as well. Just ping me in discord.

[wesleybl]

@nileshgulia1 @sneridagh I documented in PLIP situations where it is necessary to manually prefix, when using react-router's basename. Is basically what is being done in this PR.