Add internal resolution for assets in `get_url` function #2726

ZzMzaw · 2024-12-06T10:57:53Z

This PR solves issue #2598 through internal resolution for assets (i.e. using @ in front of the asset path).

It allow to retrieve an assets permalink based on its associated page and according to the language.
Documentation regarding the galery shortcode has been updated accordingly.

Beware supports for resolving assets internally has only been implemented for the get_url function.
I am not sure if it should be generalized to links as well (as get_url can and should be used to feed the href of a link).

This PR could maybe provide an answer to the issue described by the PR #2146 (a not to-be-merged PR for demo purpose only) even if it doesn't fix it where the PR points out (not sure about this at all).

Sanity check:

Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

Have you created/updated the relevant documentation page(s)?

Colocated path is required for computing serialized assets and assets permalinks. Test have been added to check colocated path format as expected by those computations.

ZzMzaw · 2024-12-07T04:43:58Z

There is something wrong with serialization of assets in sub-directories on windows.
I'll look into it but as I don't have windows to reproduce the issue locally, I'll use this PR for testing.
I may force-push several time until I'm able to fix the issue.
I'll keep you posted once it is done.

Fix assets serialization behavior which was relying on the output path instead of the source path like for pages. This behavior was making assets permalinks computation fail.

Compute assets permalinks for serialized assets based on associated page or section permalink.

Assets are duplicated for every language so permalinks should be releated to it in order to be language-aware.

Assets permalinks allows to resolve internally colocated assets as well as pages and sections.

Galery can be fixed by using internal resolution. Adding the lang as well allows to make the target URL lang-aware even if the image is just duplicated. Example file has been fixed as well even if it might not be used.

ZzMzaw · 2024-12-07T06:22:07Z

Tests are successful now, including on Windows.
It would be great if someone could test the galery shortcode on Windows.

Anyway, PR can be reviewed now.

Keats · 2024-12-18T13:40:06Z

components/content/src/file_info.rs

+            assert!(file.colocated_path.is_some());
+            if let Some(colocated_path) = file.colocated_path {
+                assert!(!colocated_path.starts_with('/'));
+                assert!(colocated_path.ends_with('/'));


Can you assert what the actual path it?

I just asserted what is really important for a colocated path which is not starting with a '/' and ending with a '/'.
I wanted to emphasize that because if you change the way you compute colocated path in the future but don't respect those two assertions, you'll break the current feature.

I definitely can split the check (one for the page and one for the section as they have different colocated path) in this test while adding a comment on what is really important to keep through time.
Would it be ok for you?

Keats · 2024-12-18T14:14:58Z

components/content/src/utils.rs

+        .map(|asset_relative_path_as_string| {
+            format!(
+                "/{}{}",
+                colocated_path.expect("Should have colocated path for assets"),


If it should have it, just pass &String for colocated_path instead of the Option

If assets is empty, I don't have any guarantee that colocated_path will be Some.
I tried to reuse the same structure as the existing code but I can add an if !assets.is_empty() to unwrap this variable once instead of doing it in the iteration.
Doing it outside of serialize_assets to get rid of the Option in the signature would require to wrap the call of this function in the if statement mentionned above.
This would result in code duplication as I would have to do it twice, one for the pages and one for the sections.

What would you prefer me to implement?

Keep it as it is

Add the if statement in serialize_assets function to avoid unwrapping the option in the iteration

Add the if statement around the serialize_assets

Keats · 2024-12-18T14:15:46Z

components/content/src/utils.rs

+) -> Vec<String> {
+    assets
+        .iter()
+        .filter_map(|asset| asset.strip_prefix(parent_path.unwrap()).ok())


Pass parent_path: &Path rather than the option if all we're doing is unwrapping it

If assets is empty, I don't have any guarantee that parent_path will be Some.
I tried to reuse the same structure as the existing code but I can add an if !assets.is_empty() to unwrap this variable once instead of doing it in the iteration.
Doing it outside of serialize_assets to get rid of the Option in the signature would require to wrap the call of this function in the if statement mentionned above.
This would result in code duplication as I would have to do it twice, one for the pages and one for the sections.

What would you prefer me to implement?

Keep it as it is

Add the if statement in serialize_assets function to avoid unwrapping the option in the iteration

Add the if statement around the serialize_assets

Keats · 2024-12-18T14:17:24Z

components/content/src/utils.rs

+        expected_assets_permalinks.insert(
+            "posts/my-article/subdir/GGG.txt".to_string(),
+            format!("{}{}", parent_permalink, "subdir/GGG.txt"),
+        );


Something like:

let m: HashMap<_, _> = HashMap::from_iter([("one", 1), ("two", 2), ("three", 3)]);

is cleaner

Thanks for the head's up.
I'll fix it shortly.

OK, I had time to fix it right away in the end.

Keats · 2024-12-18T14:18:31Z

components/site/src/lib.rs

    /// We need that if there are relative links in the content that need to be resolved
    pub permalinks: HashMap<String, String>,
+    /// A map of all assets (non .md files colocated to sections and pages) and their permalink, distributed per lang
+    /// We need that if there are relative links in the content that need to be resolved
+    pub assets_permalinks: HashMap<String, HashMap<String, String>>,


what's the reason for separating by lang? Otherwise we could just add them to the permalinks maps
Are the assets copied for each language? I can't remember

Yes, assets are copied for each language.

This will allow to have different permalink depending on the lang for the same asset:

For the default lang, path/to/the/asset.jpg would become https://yourwebsite.com/slug/to/the/asset.jpg

For the non-default lang lang, path/to/the/asset.jpg would become https://yourwebsite.com/lang/slug/to/the/asset.jpg

I tested several things such as always defaulting to the default lang but I find this solution to be more clean and future proof.
When a lang is selected, your assets point to the same base URL than your page.
In addition, even if today it is not the case, if we introduce per-lang assets, it will just require minor adaptations.

Please let me know if you prefer to have something more simple.

Compute colocated path for sections

9278bd1

Colocated path is required for computing serialized assets and assets permalinks. Test have been added to check colocated path format as expected by those computations.

ZzMzaw force-pushed the feat/resolve-internal-assets branch from 2115e03 to 3fb51eb Compare December 7, 2024 05:13

ZzMzaw added 5 commits December 7, 2024 06:23

Harmonize serialize assets for pages and sections

8ff616d

Fix assets serialization behavior which was relying on the output path instead of the source path like for pages. This behavior was making assets permalinks computation fail.

Add assets permalinks for pages and sections

faed94a

Compute assets permalinks for serialized assets based on associated page or section permalink.

Add assets permalinks per language to site

106c547

Assets are duplicated for every language so permalinks should be releated to it in order to be language-aware.

Add assets permalinks to get_url

9620795

Assets permalinks allows to resolve internally colocated assets as well as pages and sections.

Fix documentation about galery

f74f551

Galery can be fixed by using internal resolution. Adding the lang as well allows to make the target URL lang-aware even if the image is just duplicated. Example file has been fixed as well even if it might not be used.

ZzMzaw force-pushed the feat/resolve-internal-assets branch from 3fb51eb to f74f551 Compare December 7, 2024 05:31

Alexizx00789999 approved these changes Dec 13, 2024

View reviewed changes

Keats reviewed Dec 18, 2024

View reviewed changes

Fix hashmap init for assets permalink test

49f66b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal resolution for assets in `get_url` function #2726

Add internal resolution for assets in `get_url` function #2726

ZzMzaw commented Dec 6, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024

ZzMzaw Dec 19, 2024

Keats Dec 18, 2024

ZzMzaw Dec 19, 2024 •

edited

Loading

Add internal resolution for assets in get_url function #2726

Are you sure you want to change the base?

Add internal resolution for assets in get_url function #2726

Conversation

ZzMzaw commented Dec 6, 2024 • edited Loading

Code changes

ZzMzaw commented Dec 7, 2024 • edited Loading

ZzMzaw commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZzMzaw Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Add internal resolution for assets in `get_url` function #2726

Add internal resolution for assets in `get_url` function #2726

ZzMzaw commented Dec 6, 2024 •

edited

Loading

ZzMzaw commented Dec 7, 2024 •

edited

Loading

ZzMzaw Dec 19, 2024 •

edited

Loading