Convert relative src references to absolute paths #677

sijie123 · 2019-02-07T07:44:30Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Documentation update
• [ ] Bug fix
• [ ] New feature
• [x] Enhancement to an existing feature
• [ ] Other, please explain:

Fixes #598
Fixes #661

What is the rationale for this request?
Currently, authors can use dynamic references in include tags.
However, if authors use dynamic resource references in src tags for pic/img etc, these references may cause broken links when re-used. This request converts all dynamic resource references to static references, so it will work consistently across all sites and sub-sites. At the same time, this request should be backward compatible, so that authors using older workarounds should expect their site to continue working.

What changes did you make? (Give an overview)
During the preprocess stage, in addition to preprocessing includes, also preprocess media resources (e.g. pic) to convert all dynamic references to static ones with respect to the root path (where site.json is located)
Additionally, removed unnecessary use of {{baseUrl}} from the user guide, changing all such links to static references. However, I did not touch mbdf files within userGuide/syntax and userGuide/plugins, as relative references might make it confusing to write these files in the future.

Provide some example code that this change will affect:
Refer to #598
Folder structure:

root/
- index.md
- a/
- - img.png
- - subsite.md

index.md

...
<include src="a/subsite.md" />

subsite.md

...
<img src="img.png" />

New syntax changes
Authors can now also use relative src references in img, pic tags, as well as relative href attributes in a and link tags.
To use

relative src attributes: All references without a leading '/' or {{...}} are treated as relative references.
static src attributes: All references with a leading '/' or starts with {{variables}} are treated as absolute paths, and will not be modified.

Breaking changes
There should be no breaking changes. I tested against the old unit tests (I didn't modify anything) and code is passing. I have also added unit tests to ensure that future changes will retain this behaviour.
My PR might break existing documents if authors used incorrect URI statements like /{{baseUrl}}/path/to/file. Notice that this string, in the original version, essentially resolves to //rootFolder/path/to/file with 2 forward slashes. While previous versions might be lenient, my conversion requires stricter syntax and might result in broken links.

Is there anything you'd like reviewers to focus on?
Are there scenarios where this change would cause something to break? I've tested on unit tests and CS2103-website.

Testing instructions:
Replicate the folder structure as above. Be sure to include dynamic references to media resources. Run markbind serve, then view the generated webpage.

Chng-Zhi-Xuan

I found an edge case you didn't cover. If you put a <pic> component within <panel> it will not change the src.

In `index.md`	In `images/subsite.md`
`<include src="images/subsite.md"></include>`	`<panel header="### test-pic">` `<pic src="om.jpg" alt="om-pokemon"></pic>` `</panel>`

EDIT:
Checked out PR 667 instead of PR 677 for testing.

Tested on PR 677 and the above example works.

Checkout wrong branch for testing

sijie123 · 2019-02-11T04:37:27Z

I found an edge case you didn't cover. If you put a <pic> component within <panel> it will not change the src.

In index.md In images/subsite.md
<include src="images/subsite.md"></include> <panel header="### test-pic">
<pic src="om.jpg" alt="om-pokemon"></pic>
</panel>
EDIT:
Checked out PR 667 instead of PR 677 for testing.

Tested on PR 677 and the above example works.

Thanks for testing though - that was a case I didn't think of, but somehow it works :P
I'll be sure to add it to my unit tests.

damithc · 2019-02-18T15:50:42Z

How's this going?
I suppose the old syntax will continue to work even after this is merged?
Don't forget to update the docs too.

sijie123 · 2019-02-21T07:39:47Z

I've been working on SE-EDU over the past couple of weeks, so I haven't done much here.
I'll be fixing this issue for other media tags over the next week or so, and I'll write some tests for it.

The old syntax will continue to work as this will only change relative URLs, and yes, thanks for the reminder. Will update the docs too.

sijie123 · 2019-02-22T09:31:44Z

Just pushed an update to this PR.
Now fixes relative URIs in src attributes in img and pic. Additionally, also supports href attrib in a and link tag.

Also works for URIs involving variables: {{baseUrl}}/path/to/file. However, this assumes that a path starting with {{ means an absolute path. I'm not sure if this is a good idea.

I have updated the PR issue with some things I need help with, from reviewers.

sijie123 · 2019-02-22T10:15:13Z

@Chng-Zhi-Xuan something's broken right now, please don't review it yet! :)

sijie123 · 2019-03-02T13:08:57Z

I've updated the code and added unit tests.
Tested with:

Unit and Functional tests
CS2103T Website

Ready for review @Chng-Zhi-Xuan

sijie123 · 2019-03-02T13:19:41Z

How's this going?
I suppose the old syntax will continue to work even after this is merged?
Don't forget to update the docs too.

Prof @damithc I didn't update the docs in the end.
If anything, the current docs used to be wrong and the correct behaviour is "fixed" by this PR.
The current docs actually doesn't say anything about references breaking if we reuse images and other media content through including sub-sites.
This PR "fixes the bug" and allows the user to include images in sub-sites in any way they prefer. It is more intuitive: static and dynamic references both work in a way that truly "allows you to include the pages you want ... without having to change anything in the source files"

Edit: I just noticed this. I'll fix the docs up.

damithc · 2019-03-02T13:46:55Z

Perhaps also remove all unnecessary {{ baseUrl }}s in the docs site?

sijie123 · 2019-03-02T15:41:08Z

Okay :))
Done with removing unnecessary {{baseUrl}}s from the docs too.
Edited the PR message to reflect this.

docs/userGuide/syntax/links.mbdf

yamgent

Commit message: For "Convert dynamic src references to absolute paths"

Currently, dynamic resource references in src attributes for pic/img tags,
as well as href attribute for a tags are broken during reuse.

The "Currently" wording is redundant; we automatically assume that the starting paragraph is talking about the current situation before this commit happens.
It is insufficient to say that it is broken. The "why is it broken?" needs to be stated as well (the fact about outer documents when including inner documents, they do not care that paths used by the inner documents are with respect to the inner document, and NOT to the outer documents, hence the outer document needs to resolve the path rather than blindly including it).
as well as href attribute for a tags: As first I thought it was a typo, I think you meant anchor & link tags right? Since 'a' is such a short name, you should spell it out to "anchor" to avoid potential confusion.

Hence, let's convert all dynamic resource references to static references,
so that it will work consistently across all sites and sub-sites.

This is not just a problem with sub-sites actually, but it also affects any files residing inside an inner folder (Btw, just to clarify in case you misunderstood the term, sub-sites are folders with their own site.json, not all folders inside an existing website is considered a sub-site).

test/functional/test_site/sub_site/testReuse.md

yamgent

Commit message: Documentation: Remove unnecessary {{baseUrl}}

Not wrapped at 72 characters. :P

Also commit message should specify why baseUrl was used in the first place. The documentation used baseUrl because:

Relative path was not supported.
If baseUrl is not empty, not all URLs are resolved correctly by MarkBind.

These features are now available, so we remove baseUrl in this commit to make it easier to maintain the documentation.

You can also mention about not altering the .mbdf with the reason you stated in the first post.

yamgent · 2019-03-10T04:44:46Z

@sijie123 need you to resolve conflict for this PR; all PRs were affected by the recent master branch rebase.

Review is stale due to rebase required.

sijie123 · 2019-03-10T08:45:23Z

Done. Thanks for the heads-up!

test/functional/test_site/expected/sub_site/testReuse._include_.html

yamgent · 2019-03-11T08:20:44Z

Updated original post to include "fixes #661".

When including files from a sub-site or a sub-folder within the same site, Markbind only automatically resolves src references for the include and panel tags. This means that references to any other resources (such as anchor#href, img#src and pic#src) do not get resolved and converted. When these markdown files containing relative resource references are included by another file in a different directory, the links remain unchanged. From the perspective of the outer document, the references become broken and files become not found. To resolve this problem, let's convert all relative resource references to absolute ones. This allows the references to work consistently across all instances of reuse. This conversion process adds on to the existing preprocess step and is similar to how we currently resolve src references in include and panel tags.

The documentation uses {{baseUrl}} to create absolute references to many resources, including CSS files, links and images. This was necessary because we did not have features to support dynamic resource references. To ensure that our sites render consistently regardless of the page that the user is in, we manually referenced each resource with respect to an absolute {{baseUrl}}. This is no longer necessary, as we can now support relative paths. As Markbind can now automatically convert relative resource references to absolute ones, we can remove {{baseUrl}} to make it easier to maintain the documentation in the future. In this commit, all unnecessary references to {{baseUrl}} have been removed, except for mbdf files within userGuide/syntax and userGuide/plugins. These files are meant as the central reference within the user guide, and removing {{baseUrl}} from these files might make it confusing to maintain or add new documentation in the future.

Chng-Zhi-Xuan

Tested again, didn't notice any critical issues.

damithc · 2019-03-18T06:59:45Z

@sijie123 this doesn't seem to work when the file containing the relative link is included somewhere else. Is it supposed to?

damithc · 2019-03-18T07:14:10Z

@sijie123 this doesn't seem to work when the file containing the relative link is included somewhere else. Is it supposed to?

In fact, it seems to work for <img src="images/createNewProject.png"> but not for ![](images/createNewProject.png)

sijie123 · 2019-03-18T10:22:28Z

@sijie123 this doesn't seem to work when the file containing the relative link is included somewhere else. Is it supposed to?

If I understand your question correctly, are you trying to include files across different sites? (ie site_one trying to include site_two/file.md, where site_two is not a sub-site or sub-folder of site-one)
Right now, we don't have cross-site support, so the included file must be a sub-site or within a sub-folder of the originating site for relative URLs to work. Otherwise, the author will still have to use absolute paths.

@sijie123 this doesn't seem to work when the file containing the relative link is included somewhere else. Is it supposed to?

In fact, it seems to work for <img src="images/createNewProject.png"> but not for ![](images/createNewProject.png)

This however, seems to be an issue. I implemented the code with the assumption that markdown code will first be converted to html before we do any parsing. It seems that this is not the case? (@yamgent / @Xenonym: do you guys know how our parser layers work?)

damithc · 2019-03-18T10:27:46Z

If I understand your question correctly, are you trying to include files across different sites? (ie site_one trying to include site_two/file.md, where site_two is not a sub-site or sub-folder of site-one)
Right now, we don't have cross-site support, so the included file must be a sub-site or within a sub-folder of the originating site for relative URLs to work. Otherwise, the author will still have to use absolute paths.

It's within the same site, not even a subsite, but the problem is limited to only the ![]() syntax.

yamgent · 2019-03-18T10:32:36Z

(@yamgent / @Xenonym: do you guys know how our parser layers work?)

Parser._preprocess() is called before any markdown-to-html conversion takes place. It is directly called by Parser.includeFile(), which is in turn called here:

markbind/src/Page.js

Lines 776 to 796 in 40f3e7a

    
           return new Promise((resolve, reject) => { 
        
             markbinder.includeFile(this.sourcePath, fileConfig) 
        
               .then((result) => { 
        
                 this.collectFrontMatter(result); 
        
                 return this.removeFrontMatter(result); 
        
               }) 
        
               .then(result => addContentWrapper(result)) 
        
               .then(result => this.preRender(result)) 
        
               .then(result => this.insertPageNavWrapper(result)) 
        
               .then(result => this.insertSiteNav((result))) 
        
               .then(result => this.insertTemporaryStyles(result)) 
        
               .then(result => this.insertFooter(result)) // Footer has to be inserted last to ensure proper formatting 
        
               .then(result => formatFooter(result)) 
        
               .then(result => markbinder.resolveBaseUrl(result, fileConfig)) 
        
               .then(result => fs.outputFileAsync(this.tempPath, result)) 
        
               .then(() => markbinder.renderFile(this.tempPath, fileConfig)) 
        
               .then(result => this.addAnchors(result)) 
        
               .then(result => this.postRender(result)) 
        
               .then(result => this.collectPluginsAssets(result)) 
        
               .then((result) => { 
        
                 this.content = htmlBeautify(result, { indent_size: 2 });

markbinder.renderFile() is the one that is converting markdown to HTML.

In fact, it seems to work for <img src="images/createNewProject.png"> but not for ![](images/createNewProject.png)

I completely forgotten about the markdown syntax equivalents, we are doing the relative src referencing in the wrong pass.

sijie123 · 2019-03-18T10:38:34Z

😱 I think I'll have to discuss this issue with you on Thursday...
Right now I think maybe we might need to add an extra layer after markdown converts everything?
I was hoping to be able to not introduce an additional layer, but I think we don't have much choice ._.

yamgent · 2019-03-18T15:15:11Z

Yes looks like we will have to introduce an additional .then(result => ...) (I presume that is what you meant).

Then again, if a developer fears adding a new layer, that is a sign that that part of the code is too intertwined, and needs to be refactored into more logical units. 😛

sijie123 · 2019-06-26T11:08:18Z

For future reference, the follow-up issue is resolved by #826.

Chng-Zhi-Xuan previously requested changes Feb 11, 2019

View reviewed changes

sijie123 force-pushed the relativeSrc branch from 4ff88ea to d6c0d87 Compare February 22, 2019 09:27

Chng-Zhi-Xuan self-requested a review February 22, 2019 10:05

sijie123 force-pushed the relativeSrc branch from d6c0d87 to 4ff88ea Compare March 2, 2019 13:07

sijie123 changed the title ~~[WIP] Convert dynamic src references to absolute paths~~ Convert dynamic src references to absolute paths Mar 2, 2019

sijie123 force-pushed the relativeSrc branch 9 times, most recently from baee282 to 8c36434 Compare March 2, 2019 15:34

damithc reviewed Mar 3, 2019

View reviewed changes

docs/userGuide/syntax/links.mbdf Show resolved Hide resolved

sijie123 force-pushed the relativeSrc branch 3 times, most recently from f40ea57 to 7e64521 Compare March 3, 2019 05:55

yamgent reviewed Mar 6, 2019

View reviewed changes

test/functional/test_site/sub_site/testReuse.md Show resolved Hide resolved

yamgent reviewed Mar 6, 2019

View reviewed changes

yamgent force-pushed the master branch from 7f35131 to 43a048d Compare March 8, 2019 15:01

sijie123 force-pushed the relativeSrc branch from cd29626 to bf98537 Compare March 10, 2019 08:43

sijie123 requested a review from yamgent March 10, 2019 08:45

Chng-Zhi-Xuan reviewed Mar 11, 2019

View reviewed changes

test/functional/test_site/expected/sub_site/testReuse._include_.html Show resolved Hide resolved

yamgent requested changes Mar 11, 2019

View reviewed changes

test/functional/test_site/expected/sub_site/testReuse._include_.html Show resolved Hide resolved

yamgent removed this from the v1.19.3 milestone Mar 11, 2019

sijie123 added 2 commits March 11, 2019 15:36

sijie123 force-pushed the relativeSrc branch from bf98537 to 8482de0 Compare March 11, 2019 08:38

sijie123 requested a review from yamgent March 11, 2019 08:39

Chng-Zhi-Xuan approved these changes Mar 11, 2019

View reviewed changes

yamgent approved these changes Mar 12, 2019

View reviewed changes

yamgent added this to the v1.20.1 milestone Mar 12, 2019

yamgent changed the title ~~Convert dynamic src references to absolute paths~~ Convert relative src references to absolute paths Mar 12, 2019

yamgent merged commit 6119a91 into MarkBind:master Mar 12, 2019

acjh mentioned this pull request Apr 21, 2019

Unable to run markbind serve from another directory #842

Closed

Chng-Zhi-Xuan mentioned this pull request Jun 3, 2019

Add support for thumbnails using <thumbnail> #881

Merged

damithc mentioned this pull request Jun 19, 2019

Update docs for relative intra-site links support #888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert relative src references to absolute paths #677

Convert relative src references to absolute paths #677

sijie123 commented Feb 7, 2019 •

edited by yamgent

Loading

Chng-Zhi-Xuan left a comment •

edited

Loading

sijie123 commented Feb 11, 2019

damithc commented Feb 18, 2019

sijie123 commented Feb 21, 2019 •

edited

Loading

sijie123 commented Feb 22, 2019 •

edited

Loading

sijie123 commented Feb 22, 2019

sijie123 commented Mar 2, 2019 •

edited

Loading

sijie123 commented Mar 2, 2019 •

edited

Loading

damithc commented Mar 2, 2019

sijie123 commented Mar 2, 2019

yamgent left a comment •

edited

Loading

yamgent left a comment

yamgent commented Mar 10, 2019

sijie123 commented Mar 10, 2019

yamgent commented Mar 11, 2019

Chng-Zhi-Xuan left a comment •

edited

Loading

damithc commented Mar 18, 2019

damithc commented Mar 18, 2019

sijie123 commented Mar 18, 2019

damithc commented Mar 18, 2019

yamgent commented Mar 18, 2019

sijie123 commented Mar 18, 2019

yamgent commented Mar 18, 2019

sijie123 commented Jun 26, 2019

Convert relative src references to absolute paths #677

Convert relative src references to absolute paths #677

Conversation

sijie123 commented Feb 7, 2019 • edited by yamgent Loading

Chng-Zhi-Xuan left a comment • edited Loading

Choose a reason for hiding this comment

sijie123 commented Feb 11, 2019

damithc commented Feb 18, 2019

sijie123 commented Feb 21, 2019 • edited Loading

sijie123 commented Feb 22, 2019 • edited Loading

sijie123 commented Feb 22, 2019

sijie123 commented Mar 2, 2019 • edited Loading

sijie123 commented Mar 2, 2019 • edited Loading

damithc commented Mar 2, 2019

sijie123 commented Mar 2, 2019

yamgent left a comment • edited Loading

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

yamgent commented Mar 10, 2019

sijie123 commented Mar 10, 2019

yamgent commented Mar 11, 2019

Chng-Zhi-Xuan left a comment • edited Loading

Choose a reason for hiding this comment

damithc commented Mar 18, 2019

damithc commented Mar 18, 2019

sijie123 commented Mar 18, 2019

damithc commented Mar 18, 2019

yamgent commented Mar 18, 2019

sijie123 commented Mar 18, 2019

yamgent commented Mar 18, 2019

sijie123 commented Jun 26, 2019

sijie123 commented Feb 7, 2019 •

edited by yamgent

Loading

Chng-Zhi-Xuan left a comment •

edited

Loading

sijie123 commented Feb 21, 2019 •

edited

Loading

sijie123 commented Feb 22, 2019 •

edited

Loading

sijie123 commented Mar 2, 2019 •

edited

Loading

sijie123 commented Mar 2, 2019 •

edited

Loading

yamgent left a comment •

edited

Loading

Chng-Zhi-Xuan left a comment •

edited

Loading