-
-
Notifications
You must be signed in to change notification settings - Fork 653
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
Relative (prefixed root) path #3190
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This is what I mean. Also the toctree can be simplified further.
|
src/helpers/Url/Url.js
Outdated
return ( | ||
url && | ||
url | ||
`${settings.prefixPath}${url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a problem with listing blocks (and maybe in other parts).
in universalLink the url will be "flattened" several times and with your changes, i have links inside listing blocks like "/my-domain/my-domain/example-page".
Probably in universalLink code we should avoid multple flattenToAppURL calls, but maybe also in that function we should avoid to append the same string several times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more inclined towards the latest solution. This function should be idempotent imho, so it should not prepend the prefix every time it's called.
I wouldn't check if the url already starts with the prefixPath. Maybe we should only add the prefixPath if the url starts with http
?
@@ -279,6 +280,18 @@ const defaultModify = ({ | |||
}), | |||
] | |||
: []; | |||
|
|||
const prefixPath = process.env.RAZZLE_PREFIX_PATH || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Razzle docs, there should be an env var that already does this: PUBLIC_PATH
https://razzlejs.org/docs/environment-variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process.env.PUBLIC_PATH: Only used in razzle build. You can alter the webpack.config.output.publicPath of the client assets (bundle, css, and images). This is useful if you plan to serve your assets from a CDN. Make sure to include a trailing slash (e.g. PUBLIC_PATH=https://cdn.example.com/). If you are using React and altering the public path, make sure to also include the crossorigin attribute on your <script> tag in src/server.js.
process.env.CLIENT_PUBLIC_PATH: The NODE_ENV=development build's BUILD_TARGET=client has a different PUBLIC_PATH than BUILD_TARGET=server. Default is http://${process.env.HOST}:${process.env.PORT + 1}/
Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how usable those will be, as we'll need the same var inside Volto, for path adjustments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the aforementioned docs, all those vars are available in process.env at build time for the client, as well as all the RAZZLE_ vars that we already know. I have actually never tested this fact, on the other hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll give it a try, though I'm wary of this, as the PUBLIC_PATH in razzle docs show it as a full URL, while we're dealing with paths fragments in the Volto implementation. Maybe if we take public_path as an override even for apiPath...
Another problem: in control-panels, save button works fine but clicking the X, does not brings me to the list of control-panels as expected. Panels with "back" button (for example dexterity types one) works fine. |
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
I tried to redo all the necessary changes that made the prefix run with 16.3.0 to run on 16.20.4. I am trying to create a how-to replicate the work done on 16.3.0 to enable this feature on newer version of Volto, without any tests. To do that I identified the work (diff - 16.3.0, relative_path_redo) => 16.20.4
What happend:
Solved: I was missing the fact that I need to change the routes.js in the Volto site package as well. Things are working smoothly. |
@@ -0,0 +1,68 @@ | |||
--- | |||
html_meta: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
html_meta: | |
myst: | |
html_meta: |
Also indent lines 3-6 with two more spaces. The format of this changed since this PR was created.
Related to #4290 , to determine which one is the canonical and if we can close this one. |
Based on previous effort from cekk, giulia and all. I'm trying to go at this from a clean state, adding things step by step so that I understand them. I'll provide proper attributions at the end.