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

Support relative paths if options.directory starts with "." #143

Closed

Conversation

kbuffington
Copy link

@kbuffington kbuffington commented Oct 26, 2021

I noticed that relative paths didn't work correctly and that the simple fix would be to determine if the options.directory started with a period (to handle "." and "..") and then do path.join(app.getAppPath(), directory) which will spit out an absolute path. (#125)

Does not currently handle "~/" but that could be done pretty easily if needed.

@kbuffington kbuffington changed the title Support relative paths if options.directory starts with "." (#125) Support relative paths if options.directory starts with "." Oct 26, 2021
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I don't think you understood my comment in #143 (comment). I'm arguing that making relative paths relative to the app directory is not expected behavior as a user. I'm honestly not sure what the expected directory would be. I'm leaning towards enforcing an absolute path.

@kbuffington
Copy link
Author

I don't think you understood my comment in #143 (comment). I'm arguing that making relative paths relative to the app directory is not expected behavior as a user. I'm honestly not sure what the expected directory would be. I'm leaning towards enforcing an absolute path.

Sorry, I think I wasn't clear earlier. Using path.resolve, the relative paths are now based off the directory which the app was launched from (which may not be the directory the app is in) which seems to be the correct way to handle this (where else could a relative path even be relative to but the working directory?).

So if I'm in /a/b/ and I run electron ./c/. and inside the program I pass a path of ../file.dat that will be downloaded to /a/file.dat.

Personally, I don't see any particular reason to always enforce absolute paths, particularly since relative paths don't work now. Currently what happens if you specify a relative path, is that files are created at that location, but they are all 0-byte files and the download fails.

With this current PR if the user specifies a relative path and it doesn't save the file where they expect, then they can figure out how to get the relative path pointing to the location they want, or they can always use an absolute path. There's no alternative the other way, so why not allow them?

@sindresorhus
Copy link
Owner

There's no alternative the other way, so why not allow them?

Because silent ambiguity is worse than a loud error.

@sindresorhus
Copy link
Owner

if the user specifies a relative path and it doesn't save the file where they expect

I don't think it's a if. It would not save where the user expect as there's really no expectation for a relative path unless it's documented in the app, in which case, the app can handle resolving to an absolute path.

@kbuffington
Copy link
Author

kbuffington commented Nov 10, 2021

Because silent ambiguity is worse than a loud error.

Right now there's no "loud errors" being thrown, or even any errors of any kind. Instead, it just acts like it works, but actually just creates 0-byte length files with the filename you specify, sometimes in the correct relative locations. How is that better? In case you don't believe me:

image

The only indication that anything went wrong to the user is that the download badges don't clear in the current relative path case.

I also don't understand why you think there's any ambiguity. If I attempt to launch an app from the command line with a relative path, I know it's from where I currently am. If I pass a relative path to an app, I think pretty much everybody would also assume that relative path is also from the directory I'm currently in.

In the case that you feel there is some ambiguity, why not just document it? Then there's zero ambiguity and users can make up their own minds whether they want to use relative paths or not. Then you won't have an app that silently fails in confusing ways if someone passes a relative path.

@kbuffington
Copy link
Author

kbuffington commented Nov 10, 2021

I see that your latest release now throws an error if passed a relative path, so sorry for wasting everybody's time.

@sindresorhus
Copy link
Owner

If I attempt to launch an app from the command line with a relative path

Most users don't launch apps from the command-line.

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