laosb / ghos3

A modernized AWS S3 storage adapter for Ghost.
Other
34 stars 12 forks source link

Normalize path #14

Open dyadyaJora opened 8 months ago

dyadyaJora commented 8 months ago

Hello!

Thank you for supporting this s3 storage adapter! Could you please take a look at this request: I want to add step with replacing \ with / in order to get ability to run ghost instance with this plugin on windows for local debuging purposes. While fs.join and other functions from fs package return by default path this backslashes \, so all links to aws become broken. This PR fixes such issue. Please accept this PR if you find it useful.

laosb commented 7 months ago

I see. That means any "path" for S3 side must use /, but using a regex to replace all \ looks wrong to me. I'll investigate this further before I can merge this.

laosb commented 7 months ago

After some investigation, I believe the correct way to do this is to change the import from path to path/posix, to always use POSIX style path separator, as in this adapter we are only dealing with S3, which always use POSIX style.

https://nodejs.org/api/path.html#pathposix

dyadyaJora commented 7 months ago

Please use path/posix instead of patching the behavior using a regex, as outlined in my last comment.

I tried replacing path with path/posix, but it doesn't help fix the issue. The root cause is the usage of path instead of path/posix in functions from the parent class StorageAdapter. https://github.com/TryGhost/Ghost-Storage-Base/blob/main/BaseStorage.js#L2

In order to fix it with path/posix import, it is neccessary to override every method with path.join from BaseStorage, which doesn't look good.
May be there is another clean solution, but unfortunately I don't see it.

laosb commented 7 months ago

Hmm, then there's no way to do it in a clean way in the scope of this project.

Maybe you can raise an issue upstream and see if they can provide a better way to handle this?

I imagine they can require an optional pathJoin method to be implemented in StorageAdapter, and fallback to use require(path).join if the StorageAdapter doesn't implement one.

If that sounds good please pitch this to Ghost and let's see if they're willing to provide that infra for us (and pretty much every Object Storage service based adapters). Otherwise we may have to do this behind an option, as \ is actually a valid character for folder / files on POSIX and we will be breaking others' setup if just do the replace (though I don't imagine there will be more than a single digit number of people doing this.)