Skip to content
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

Normalize path #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Normalize path #14

wants to merge 1 commit into from

Conversation

dyadyaJora
Copy link

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
Copy link
Owner

laosb commented Mar 22, 2024

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
Copy link
Owner

laosb commented Mar 28, 2024

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

Copy link
Owner

@laosb laosb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dyadyaJora
Copy link
Author

dyadyaJora commented Mar 31, 2024

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
Copy link
Owner

laosb commented Apr 2, 2024

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants