search

httptoolkit / httptoolkit-ui

The UI of HTTP Toolkit
https://httptoolkit.com
GNU Affero General Public License v3.0
282 stars 106 forks source link

Fix: Make Electron apps available on macOS again #80

Closed datenreisender closed 1 year ago

datenreisender commented 1 year ago

Requires httptoolkit/httptoolkit-desktop#60, done for httptoolkit/httptoolkit#329

datenreisender commented 1 year ago

@pimterry I addressed the issues regarding the wrong type of nativeFile and then wrong "win" in two commits. Squashing them into the first commit would also be fine for me, I just didn't know whether you prefer to keep them separate in the PR to make it easier to see them during review.

Now I see two more aspects:

  1. Use the native file open dialog in uploadFile on all platforms
  2. Adjust the help text on macOS

Use the native file open dialog in uploadFile on all platforms

While I think it is a good idea to do it, I would rather do that in a PR on its own. This has too many other things to be considered (e.g. handling when mime types are passed to uploadFile, maybe setting a better title for the different file open dialogs) that I would rather not do it in this PR here.

Adjust the help text on macOS

The text in https://github.com/httptoolkit/httptoolkit-ui/blob/a6556a083f35c16d61a2c4a5a56ed92de6bbae77/src/components/intercept/config/electron-config.tsx#L196-L198 isn't really correct anymore now, because with the native file open dialog, people cannot really select app bundle itself any longer. And from what I understood, the function to determine the executable within the app bundle didn't work very well before anyhow, which is why you disabled the Electron injector on macOS in the first place?

I have three ideas for this:

  1. Shorten the text to For .app bundles select the executable itself, typically stored in Contents/MacOS inside the bundle..
  2. Leave it as it is for now.
  3. Leave the text mostly and also enable the selection of directories, so that the app bundle itself can also again be selected, by adding 'openDirectory' to https://github.com/httptoolkit/httptoolkit-desktop/pull/60/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R593

I prefer 1.

What do you think?

pimterry commented 1 year ago

Thanks for those fixes @datenreisender.

from what I understood, the function to determine the executable within the app bundle didn't work very well before anyhow, which is why you disabled the Electron injector on macOS in the first place?

This is worth clarifying, because that's not quite right. The problem is that if you don't enable treatPackageAsDirectory then when you select a .app file, MacOS doesn't select the directory, instead it tries to find the single file you really meant.

It does that by searching within the entire app package automatically, which is very slow and freezes everything for a while, and usually picks the wrong file at the end anyway. That all happened before we even received the selected file, so there was nothing we could do about this.

If the .app is actually selected as a directory though (i.e. so the server receives "/Applications/abc.app" as the target to intercept) then it's all fine and everything will work great. The logic powering that is here (then called here) - it just reads the .app bundle metadata to get the binary path, and then runs that instead.

This is a good point though, because it highlights that we're not currently supporting that here, but as you point out we could, very easily! I think we should - I'll add a note on the desktop PR.

For your two questions:

1.

I'm happy to enable this Mac-only to start with, and expand support for other features separately, but in that case we do still need to make sure that the API (as defined by the desktop PR) is carefully designed & set up for expansion.

Once any of this is shipped it stays shipped forever (especially in the desktop) so we'll have to remain compatible with it in future. We can't ship a nativeFile.open API that won't work for future expansions cases (or at least, if we wanted to change it incompatibly later we'd have to ship a totally separate API too and support both, which would be a bit annoying). People keep running weird desktop versions for ages, and even downloading & installing them from scratch years later.

I think that's actually fine though - we can handle this by making this open method specific to selecting applications, fully handle that (I think the logic for that specifically is pretty simple) and we can use it here just for that case and delay all other open-file cases to a separate method we'll create later.

I'll add some comments on the desktop PR to explain what I mean in a second.

2.

For the text you mentioned, we need to be a little clever, because the current text is still correct & valid for anybody using HTTP Toolkit from a browser without Electron (where they enter the app path manually with a prompt). That's who the test is currently intended for really, since it's not possible in Electron right now, and that case will still exist in future too. We could detect the different cases, but I think it's easiest to just be very generic with something like:

For .app bundles, you can intercept either the bundle (the .app directory) or the executable itself, typically stored in Contents/MacOS inside the bundle.

(Assuming the changes I'm about to propose on the desktop side, so that works)

Does that make sense?

datenreisender commented 1 year ago

@pimterry: Matching https://github.com/httptoolkit/httptoolkit-desktop/pull/60 I also did the needed changes here.