utils: perform portable path sanitisation of URLs #721
Conversation
src/utils/data.js
Outdated
| @@ -56,7 +56,7 @@ export async function fetchData(path, params = {}, options = {}) { | |||
| } | |||
|
|
|||
| function createDataPath(path) { | |||
| const dataPath = path.replace(/\/$/, ''); | |||
| const dataPath = path.replace(/\/$/, "").replace(/[<>:"\/\\|*]/, "_"); | |||
There was a problem hiding this comment.
Is this change strictly needed for the corresponding swift-docc PR to work?
Generally the renderer relies on the URL path matching the name of the JSON file on-disk. I am a little concerned that this wouldn't be a backwards-compatible change—newer versions of the renderer wouldn't work anymore with older content that hasn't been compiled with the newer version of DocC since the URL and file would no longer match.
There was a problem hiding this comment.
Yes, this is required as the DocC change will change the on-disk name of the files. This is one of the few final pieces needed to support Windows.
I couldn't figure out how to do an auxiliary file to indicate the disk layout. I was imagining that we could have DocC write out a settings.json (something like {portablePaths:true}) and we could use that to make this conditional. If you have pointers to where I could read such a file on the SPA side, I could look into the appropriate changes for the DocC side as well.
There was a problem hiding this comment.
Hm. I'm not sure I have a good answer at the moment, and I'll need to think about this a little more. The backwards compatibility would be important in environments where the renderer version may be updated independently of the version of DocC used to compile the render JSON.
Apologies for the delay! I really appreciate you taking the time to try and improve the experience here for DocC on Windows.
There was a problem hiding this comment.
Discussed this with @d-ronnqvist and he recently commented on the forum post as well.
I think the theme-settings.json file would be the only solution here that doesn't introduce a backwards compatibility issue or major breaking change. We could potentially add a feature flag to that file which this logic here can conditionalize itself. It's not an ideal solution, but I'm not sure we have a better way yet.
There was a problem hiding this comment.
Okay, so, you want to put it into the theme settings and not an application level settings file? The piece that I'm unsure about is how to get the configuration value itself still.
There was a problem hiding this comment.
Yeah, I think that might be our best option at the moment, although I would agree that it is not an ideal solution.
Unfortunately we don't really have a system for an application-level settings file that can be configured dynamically with individual DocC builds right now, because DocC-Render is pre-built and distributed as static assets as opposed to being built when DocC is invoked to compiler docs.
To get a configuration value from the theme-settings.json file, you can utilize the getSetting function from src/utils/theme-settings.js.
Example usage:
There was a problem hiding this comment.
I took a look at the DocC path for theme-settings.json. It seems that the user may or may not provide a value. Would we just update the theme-settings.json default and then base the behaviour on that?
There was a problem hiding this comment.
You're right that the value might not be provided and even the file itself is not required by the user.
I think if we were to make a new feature flag for the DocC compiler for this new behavior, it might make sense to update it to create a theme-settings.json file if one hasn't been already provided by the user—then DocC can update/add this feature flag to that JSON file without the user having to manually do it for the renderer as well.
(Sorry, went on a vacation and missed this comment while I was out)
Some file systems have restrictions on character sets which are valid file name characters. Add a filter for the Windows file system character set restrictions. We replace them with `_` to match the behaviour in the DocC bundle generation after apple/swift-docc#668. Conditionally enable the new portable paths based upon a setting in `theme-settings.json` to provide a means for compatibility.
|
CC: @theMomax |
Some file systems have restrictions on character sets which are valid file name characters. Add a filter for the Windows file system character set restrictions. We replace them with
_to match the behaviour in the DocC bundle generation after apple/swift-docc#668.