Skip to content
This repository has been archived by the owner on Jul 14, 2023. It is now read-only.

update Electron and some other dependencies #30

Merged
merged 26 commits into from
Mar 28, 2018
Merged

update Electron and some other dependencies #30

merged 26 commits into from
Mar 28, 2018

Conversation

shiftkey
Copy link
Collaborator

@shiftkey shiftkey commented Mar 22, 2018

You're probably wondering what I'm doing here. @johndbritton was looking for someone with Electron experience to help out with supporting this app and helping to get new features in. I work on the Desktop team at GitHub, so I figure I'd be able to help out.

Also, fair warning: I've not used the Classroom app before this, so it'd be great to get some help to confirm I haven't broken anything in here as I've been tidying up.

So, what have I done?

  • Update Electron to 1.8.4
  • Ensure native modules are compiled for use in Electron - that's the .npmrc file at the root of the repository, and should be bumped in time with Electron
  • Use electron-mocha instead of jest - Jest is cool, but the clone tests using nodegit should be run within an Electron app, and electron-mocha is designed for just that. I couldn't find an equivalent test framework for Jest to do this that was well-supported, but most of the remaining changes were around assertions.
  • Use vanilla React wherever possible - I'm not familiar with recompose, but I think it's fine here to just write some plain React components and 🔥 that dependency
  • Deprecate react-spinjs for react-spinners - react-spinjs hasn't been updated in ~2yrs, so I just switched it over to something better supported. There's probably some styling issues here, as I wasn't sure how to trigger it and see the animation, but we can deal with that later.

TODO

  • [ ] there's one more test (currently skipped) that relies on nested actions - it'd be nice to get that working but I'm not sure how to spy on the actions sent when they're nested in this way moved to investigate enabling submission-clone-all-test #34
  • [ ] we're currently running an old version of react-router - I'm not familiar with how to migrate fully, but it seems to work and I'm okay with punting that work to later because the migration path seem complex. moved to upgrade react-router to v4 #33
  • wireup Travis CI for macOS (Linux is passing) to validate both platforms
  • test the packaged app works
  • [ ] cloning the default values requires credentials - does the app need to auth and store a token to track that? moved to cloning repository requires token (and OAuth app?) #32
  • [ ] Electrons applications now need to be signed on macOS to update correctly, so the updater generates an error at runtime moved to setup codesigning and create new release #31

@shiftkey
Copy link
Collaborator Author

Rather than drag this massive PR out further, I've opened up issues to address the remaining work that I'm not sure about. @johndbritton I don't seem to have the magic merge button - would you mind doing the honours?

@johndbritton johndbritton merged commit 3fe64a3 into github-education-resources:master Mar 28, 2018
@johndbritton
Copy link
Contributor

@shiftkey merged, also giving you access to merge your own pull requests now.

@shiftkey shiftkey mentioned this pull request Mar 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants