Galooshi / happo

Visual diffing in CI for user interfaces
504 stars 16 forks source link

Move to Lerna project structure #183

Closed lelandrichardson closed 7 years ago

lelandrichardson commented 7 years ago

to: @lencioni @trotzig

Overall, this PR breaks the project into 5 packages:

The config structure is now changed so that it requires a targets array of "target" objects. A target option has the following shape:

type HappoTarget = {
  name: string,
  run(): Promise<RunResult>,
  debug(): Promise,
};

The CLI commands that change as a result of this PR are:

happo run [<target>]

runs happo with the passed in target name. If target is omitted, all targets run and are merged into a single result

happo debug <target>

runs debug on the passed target name.

trotzig commented 7 years ago

The config structure is now changed so that it requires a targets array of "target" objects.

Would it make sense to use an object instead of an array here? I feel like that would be more straightforward.

module.exports = {
  targets: {
    firefox: firefoxTarget({
      viewports: { ... },
    }),
    reactNativeIOS: reactNativeTarget(),
  }
}

Oh, but the name is probably useful down the line when generating diff pages for multiple targets? So having it attached to the target itself makes things easier.

Also since the method creating the target is expected to return an instance, maybe it would be better to have targets as classes?

const FirefoxTarget = require('happo-target-firefox');

module.exports = {
  targets: [
    new FirefoxTarget({
      viewports: { ... },
    }),
  ],
};
lelandrichardson commented 7 years ago

@lencioni @trotzig I moved some things around a bit more. The project now runs and works as far as i can tell. It looks like right now it's consistently marking the 2nd example in the example project as having a diff though. is that expected?

I ended up moving the example project into the packages directory as it makes it easier with all of the automatic symlinking that lerna does for us.

The way I had set up the config in happo-core was causing a bunch of circular dependencies because the config is a runtime JS file which would probably require a happo library (like happo-target-firefox) which would in turn depend on config from happo-core.

In order to fix the circular dependencies, I made the config object just a container for an injectable singleton config.

lelandrichardson commented 7 years ago

Also since the method creating the target is expected to return an instance, maybe it would be better to have targets as classes?

Good call. Updated to reflect this.

lelandrichardson commented 7 years ago

Would it make sense to use an object instead of an array here? I feel like that would be more straightforward.

I think having the target have a "default" name works fine, and then allows users to define more than one of the same target if they like. The expense is an array lookup in the CLI, but i don't think that matters much.

trotzig commented 7 years ago

The project now runs and works as far as i can tell. It looks like right now it's consistently marking the 2nd example in the example project as having a diff though. is that expected?

Yep, it has a Math.random() in its rendering.

trotzig commented 7 years ago

I'm trying this out locally, and so far I haven't run into any issues. 💯

Just to make sure I understand things correctly, this is what I did to try things out:

  1. Checked out your branch
  2. Ran $(npm bin)/lerna bootstrap
  3. Cd:ed into packages/happo-example-project and ran happo run firefox
  4. Watched things work
  5. Made some changes in packages/happo-target-firefox
  6. Re-ran happo run firefox in the example project
  7. Realized I hadn't started the babel watcher, doh.
  8. Ran npm run babel -- --watch from within happo-target-firefox
  9. Re-ran happo run firefox
  10. Saw my changes in action.

Great work!

lelandrichardson commented 7 years ago

@trotzig yeah, that should do it. I tried to set things up so we didn't need to cd into any directories. From the root directory you should be able to:

  1. ./example <command> should run happo <command> in the example project
  2. npm run watch should watch all of the projects at the same time
  3. lerna run build should build all of the projects at the same time
  4. npm test should run the tests on the whole project

Perhaps i should make npm run build work from the root so that those commands are symmetric

lelandrichardson commented 7 years ago

@trotzig @lencioni made npm commands for watch and build so people don't necessarily need to know to use lerna now

Will start working on tweaking the readme

lelandrichardson commented 7 years ago

@lencioni @trotzig I added a happo-uploader-s3 package, and updated some of the READMEs. PTAL!

lelandrichardson commented 7 years ago

Note: i'm thinking about adding a gitbook documentation site in a followup PR, as happo will begin to have a larger surface area with more targets/uploaders/ci etc.

lelandrichardson commented 7 years ago

@lencioni agreed. I think w/ tests passing and no glaring "we should not do {x}" or "we should fix {y}", we should merge and maybe hold off on publishing for a little bit while we continue to smoke test. What do you think?

lencioni commented 7 years ago

@lelandrichardson that sounds good to me.