search

homotopy-io / homotopy-webclient

https://homotopy.io
26 stars 5 forks source link

Use `this` to refer to global object. #55

Closed joliss closed 5 years ago

joliss commented 5 years ago

This fixes a "window is not defined" error that prevented hot module replacement from loading correctly for me. See also https://github.com/webpack/webpack/issues/6642#issuecomment-370222543

This gets the client to load for me, though it's still not quite working properly. (I'll post a separate issue in a moment.) So I recommend you try running it yourself before merging this, just to make sure it didn't inadvertently break something.

jamievicary commented 5 years ago

This is really fantastic Jo, it will make such a difference to the efficiency of the development.

jamievicary commented 5 years ago

I'm not very familiar with pull requests, and I'm a bit confused about them in some ways. Could you help me out with a few points.

wetneb commented 5 years ago

Jumping in as this is generic GitHub stuff, so for once I hope I can help :)

  • How can I test out your code locally before merging?

You need to fetch and check out the branch where the PR was made from. GitHub shows the commands required for that on the page for the pull request (if you click the view command-line instructions link next to the Merge button) but if you use git through some front end you will need to translate that to your UI.

  • A pull request might be addressing an issue in the main issue tracker, but then it's not obvious where we should having the discussion; on the comment thread for the pull request, or for the issue.

Generally you can link the PR to the issue (by mentioning either of them with their hash-number), so that it is easy to jump back and forth between the two. It's true that the line between the two can be thin, but usually you would use the issue to discuss what the problem is with the current situation (what is the desired behavior) and the PR to discuss whether the proposed code/doc changes look satisfactory or not.

  • Why not just make edits directly to master, or to a branch off master?

It is possible do open pull requests for branches that are in the same repository - you do not need to go through a fork for that (but for external contributors without push access to your repository, forking is the only solution). The advantage over directly pushing things to master is that it gives an opportunity to review and discuss changes. It also gives a way to group commits in a high-level way so as a team it is easier to keep track of what was changed and why (so it's desirable even if all the contributors are trusted).

  • After I merge the pull request it says "the joliss:webpack branch can be safely deleted". Should I typically go ahead and do that? Will it affect your ability to make future updates to this pull request? (Is "updating a pull request" even a thing?)

If a contributor pushes further commits to a branch that was already merged through a pull request, nothing will happen - they will need to open a new pull request to get their changes merged. So it is common practice not to re-purpose branches: one branch = one PR. I personally delete periodically the merged branches in the repositories I work with (but I would not delete branches in forks owned by others, as in this case).