httptoolkit / httptoolkit-desktop

Electron wrapper to build and distribute HTTP Toolkit for the desktop
https://httptoolkit.com
GNU Affero General Public License v3.0
606 stars 83 forks source link

Fix npm start:dev when httptoolkit-server dir does not exist #12

Closed evanrolfe closed 4 years ago

evanrolfe commented 4 years ago

Does this fix seem ok to you? It now works regardless of whether or not you have the httptoolkit-server folder in the project root. I also got rid of the rm -rf delete because it seemed like quite an aggressive command to run without giving the user any warning and I didn't see why it was necessary to delete the entire folder if we just want to write a couple bin files? I might not know the full story though?

claassistantio commented 4 years ago

CLA assistant check
All committers have signed the CLA.

pimterry commented 4 years ago

Hmm, interesting. I can see the argument for avoiding rm -rf, but deleting the folder is what ensures that setup-local.js script run by npm start works properly. That script checks whether the server is updating by looking at the package.json there and doing nothing unless it's outdated. If we leave that in place, then going npm start; npm run start:dev; npm start will leave you in a broken state.

I think we can manage this though - can you extend the script here to also delete httptoolkit-server/package.json? I think that should be sufficient to ensure npm start reinitializes the directory.

On the build failures, don't worry about the Travis Mac build or Appveyor. Both the Windows & Mac builds require signing certificates that can't be included in PRs for security reasons, so the app doesn't build. The Linux build passes fine though, so that's fine!

I do need you to click the button to sign the CLA though. It's a couple of paragraphs, and effectively the same as the CLAs from Ghost et al. It just formally confirms that you're licensing your contributions to the project, and that you own the rights to do that, so nobody is going to sue me for including them years into the future. Open source business is complicated :smile:

evanrolfe commented 4 years ago

@pimterry take a look at the latest commit, I decided just to keep it simple. setup-local.js also uses rm -rf so I didn't think there was much point in just removing that from setup-dev but not setup-local.

Also I've tried signing that license but it just doesn't work. I tried signing it twice from the link I got sent by email, but both times when I clicked "Sign in with github" it just redirected me back to the same page asking me to sign in.

Now when I click through from this PR on github, it lets me sign in, and I click "Sign this document" and it says "thank you the document is signed" but as you can see from this PR it still says "Agreement is not signed yet". I think that license signing system is completely broken I'm afraid!

pimterry commented 4 years ago

Thanks @evanrolfe, that code all looks good to me! Tidy fix, very nice :+1:

The CLA problem is because your local git isn't configured to match your github account. More specifically, the email address you've attached to your commits isn't in the list of emails you have on github.

You can see that if you look at the commits tab on this PR: the avatar isn't your github avatar, and the name doesn't link anywhere, unlike most commits elsewhere. The CLA signing process is attached to github users, so that means it can't connect you signing the CLA to the commits themselves.

To fix that, you can either add the email you've used locally on github (git log will tell you what that is), or you can edit the email attached to the commits to match your github email by doing an interactive rebase, editing each commit, and running git commit --amend --author "New Author Name <email@address.com>".

pimterry commented 4 years ago

Hi @evanrolfe, I'd love to get this PR wrapped up. Just needs you to add the email you use in git to your github account (here), or update your commits to use one of your github emails, and then we're all good. Let me know if I can help at all.

evanrolfe commented 4 years ago

@pimterry sorry for the delay. The CLA has been signed now!

pimterry commented 4 years ago

No problem at all, thanks! Merged :+1: