Notes on a major refactor

After a month and a bit, I am nearing the end of a major refactor to Firefox Accounts. Firefox Accounts is an authentication system that ties together Mozilla services like Firefox Sync, Marketplace, and Hello.

Firefox Accounts is accessed via both native web flows and embedded in browser chrome. In total, 5 different APIs exist to access Firefox Accounts.

  1. Browsing directly to https://accounts.firefox.com.
  2. Via about:accounts for Firefox Sync. (called the FxDesktop Channel).
  3. Via Firefox Hello (Called WebChannel).
  4. Via a traditional OAuth flow. (Called OAuthRedirect).
  5. Via an IFRAME embedded in an RP. (Called OAuthIframe). This will be used by Marketplace.

Each of these can affect the flow in subtle and not so subtle ways.

For example, by default, users who successfully sign in from the /signin screen are redirected to the /settings. However, when Firefox Accounts is used to drive Firefox Sync, users who successfully sign in are shown a page controlled by native browser chrome. To avoid the possibility of a rapid flash of the /settings screen before the native browser screen, the redirect to the /settings screen is prevented.

Each of these 5 mechanisms were added ad-hoc; logic is littered throughout the code.

Adding the IFRAME proved to be the straw that broke the camel's back. The IFRAME PR was a nasty tangle of funk. Instead of merging as is, we decided the current approach was untenable and we had to clean things up.

The management of each of these 5 mechanisms was littered throughout; consolidating and refactoring touches a large portion of the system. This refactor is huge. It's tricky. There is a lot to think about, and getting it wrong means people can't sign in to the service they want to use.

Here are some notes and thoughts from the process.

  • Read Refactoring by Martin Fowler and Kent Beck. It's amazing.
  • TEST, TEST, TEST! Unit tests alone are not enough. Functional tests alone are not enough.
    • Unit tests help with local functionality and ensure inter-module APIs work as expected. Unit tests do not test the whole system.
    • Functional tests save your ass. Once you have teased apart your modules into something more coherent and logical, functional tests will ensure you haven't deviated from the expected behavior in subtle ways. Regressions will occur where there are no functional tests. Before a single line of code is modified, identify flows without functional tests and add them. NOW.
    • Unit tests also save your ass. Functional tests ensure the system works as expected, but they do not tell you in which module or function an error in logic occurs. While refactoring, failing unit tests act as a beacon to guide you to sections of code in need of an update.
  • On a large system with several developers, it's very hard to know what all parts of the system do. If the other developers are still around, their knowledge should be relied upon to smooth the process.
  • Moving too fast is a recipe for disaster.
  • Open a Work In Progress (WIP) PR early to solicit feedback throughout the entire process.
  • When introducing a new API(s) to replace an old API, run the new and old APIs in parallel until all functionality is migrated to its new spot. Remove the old API when all functionality is migrated.
  • Check in often, squash rebase when nesessary.
  • Write good commit messages when checking in the code. A series of commit messages makes the final squashed commit message easier to write.
  • Use git branches to your advantage.
  • Merge new functional tests before the final refactor PR to help keep the final diff size down.
  • It's easy to get carried away while doing a major update and want to change everything. Every change makes the reviewer's job more difficult. Make tangential changes in a separate PR.