Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate with Happo.io #1382

Open
wants to merge 19 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@trotzig
Copy link
Contributor

commented Sep 29, 2018

Integrate with Happo.io

Happo.io is a screenshot testing service. It is used to find bugs,
visualize change and provide a visual log of your application's
components. It can be used to take screenshots in different browsers.

I've been using react-dates for a while when stress-testing happo. One
of the things that have been most tricky is that react-dates relies on
measuring elements in the DOM as part of rendering the UI. This is an
issue for default Happo, because it uses JSDOM to prerender components,
then collects the HTML and CSS and sends it to the happo.io to take
screenshots. JSDOM can't measure things. The pre-rendering phase is
important for speed and security, and is something that would be less
than trivial to bypass. So, instead of bypassing pre-rendering I've
made it pre-render in a real browser. The happo-plugin-puppeteer npm
module injects a puppeteer-powered (uses Chrome) pre-renderer to Happo.
And by using that module, we're good to go for react-dates.

Another important module here is happo-plugin-storybook. This plugin
converts all storybook stories to happo examples. This means we can keep
using Storybook and we'll get screenshot testing "for free". I picked
the .storybook-css project and disabled a few decorators/addons in the
storybook config.

For now, we'll be taking screenshots in Chrome in two viewports:

  • 900x600 ("desktop")
  • 320x640 ("mobile")

Going forward, we may choose to add other targets/browsers as well. But
to begin with, I want to make sure things are working smoothly before we
expand.

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2018

Here's what the current set of screenshots look like: https://happo.io/a/32/report/dev-267bf0c808a57df525b5 . There are a few known issues, some of which I'll work on fixing upstream and some that I think we'll just have to live with:

  • Examples in non-english locales don't look right. This is a happo.io bug, I'll look into that soon.
  • Some examples require a user action (like a click) to reveal more. I thought about auto-clicking and such but decided it wasn't worth it. There are plenty of examples of the date picker being shown anyway.

In order for this to get integrated with CI, I'm going to need help with a few things:

  • Configure happo.io API keys in Travis (EDIT: I took care of that myself)
  • Add happo to the Travis run (EDIT: I did that too)
  • Install the Happo Github app for the repo
@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2018

It looks like I also need help preventing failures in Node 4: https://travis-ci.org/airbnb/react-dates/builds/435084746?utm_source=github_status&utm_medium=notification. Happo doesn't support Node 4, and a dependency we use (puppeteer) is failing to install.

@ljharb

This comment has been minimized.

Copy link
Member

commented Sep 29, 2018

You might have to make puppeteer an optional dependency then :-/ binary deps are a big hassle, i'm afraid.

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

@ljharb I made happo-plugin-puppeteer an optional dependency here and it seems to do the right thing (ignore puppeteer install failures). Let me know if you think that's the wrong approach. And yeah, binary dependencies are a hassle.

@@ -136,6 +135,9 @@
"react-dom": "^0.14 || ^15.5.4 || ^16.1.1",
"react-with-direction": "^1.3.0"
},
"optionalDependencies": {

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

ah, i misunderstood :-/ this would make it an optional runtime dep of react-dates - there’s no optional dev deps. The only way to make this work is to either a) have it be an optional dep of another package that is a dev dep here, or b) don’t put it in package.json at all and manually install it in travis.yml, or c) find an alternative package

This comment has been minimized.

Copy link
@trotzig

trotzig Sep 30, 2018

Author Contributor

Ah, of course. It looks like I can use PUPPETEER_SKIP_CHROMIUM_DOWNLOAD to prevent that error but still have it listed as a regular (dev) dependency. I just have to figure out how to get it working without the auto-download. There might be a few trial-and-error commits coming up in this PR, bear with me...

This comment has been minimized.

Copy link
@trotzig

trotzig Sep 30, 2018

Author Contributor

Looks like that didn't work...I'll probably remove it from package.json then and just install it when it's needed.

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

We could set that env var in Travis.yml for the appropriate jobs, in either direction

This comment has been minimized.

Copy link
@trotzig

trotzig Sep 30, 2018

Author Contributor

Well, it turns out that the failure during puppeteer install was that the puppeteer's install.js file couldn't be parsed. So that env variable made no difference. I went with alternative (b) and installed it when needed instead.

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

Can you file a bug upstream? It's fine if puppeteer has a node requirement but its install script should work on everything.

@trotzig trotzig force-pushed the trotzig:happo branch 2 times, most recently from 9a36298 to 6f82361 Sep 30, 2018

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Okay, I think I'm at a state where all I need is help with installing the Happo Github app for the repo.

.travis.yml Outdated
@@ -18,9 +18,12 @@ script:
- 'if [ "${TEST-}" = true ]; then npm run tests-only ; fi'
- 'if [ -n "${KARMA-}" ]; then npm run tests-karma ; fi'
- 'if [ -n "${COVERAGE-}" ] && [ "${TRAVIS_BRANCH-}" = "master" ]; then npm run cover; cat ./coverage/lcov.info | ./node_modules/.bin/coveralls ; fi'
- 'if [ -n "${HAPPO-}" ]; then npm install --no-save happo-plugin-puppeteer && INSTALL_CMD="npm install && npm install --no-save happo-plugin-puppeteer" npm run happo-ci ; fi'

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

slightly more concise? INSTALL_CMD="npm install && npm install --no-save happo-plugin-puppeteer" && $INSTALL_CMD && npm run happo-ci?

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 1, 2018

Author Contributor

Done!

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 1, 2018

Author Contributor

I had to roll that back. I tried multiple versions of this but I couldn't get it right.

.travis.yml Outdated
env:
global:
- TEST=true
- secure: "lv5bnKRHd3A0tUequn7CrpKLkaZjVc1YmY/MzvX53wjqIwP4llpIil9XqeVYoG/VDGcf810XktZe2QeAuFUTXIZhPGQRXcWsbKbHVNmKSFQFDtXqEpB9NGw4NBCytK2ZLVXhRfrIcHm6Ltr0CSa+mO/KHtk9bE/on/Itjc0aB5b7Y1gVVHTfyZ3QOjIsPUenLRsY1tn1rUJ0meFlogPFIBbcnK+GPDL2oHsiVIOjCfBauIcRlNP7XZggFhZbHARZG7bTDr+YBuybAIBCVnex5Av7ho5EHu8FTjNrGXZyKF5IOMjqJla8UvOAoEnNe5k98O+Wx9ehlObUYGcr9szV9zGHvp0Wo+sflHdWYy5IOQAp+AQqjIz8J18ia6q0Zm9TBL5h2OEx/XwKzp4Q+F0XeTL7USnsfLuVdRBfEi4fOq/ucXnmOOJNJnBl8Q1IW5NYwF0nL1sW2jNVnQsp6a0sjLs0QWHEs1J0OMyru0vsWCJfkFQn1ozla62EzKClNW0dAvRJcHsOCD2JWCzToM3HysghPkT1jAZvfrW1ndBDYCipetgSx9AoYb2jYxrxFx3BoZBd23zIy5WDa0V/HkwijV04GEt0/V/rXF3pQzEaKE07sY8rjeTHNew/bmmNcid3/MsPv/8596j6ir4QmuxaHi/kP5ZTQWjtjytUADB7HqU="

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

can you elaborate on what these secure env vars are called, and what their purpose is? if we need to update them later, how can that be done?

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 1, 2018

Author Contributor

Ah, good callout. I meant to do this but forgot. I'll comment inline.

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 2, 2018

Author Contributor

I added the name of the variable in .travis.yml, then used the commit message to explain things. Anyone ending up git-blaming these should land in this commit: ecb728a.

Show resolved Hide resolved .travis.yml
@@ -25,6 +25,8 @@
"test": "npm run build && npm run tests-only",
"storybook": "start-storybook -p 6006",
"storybook:css": "npm run build:css && start-storybook -p 6006 -c .storybook-css",
"happo": "npm i --no-save happo-plugin-puppeteer && happo",

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member
"prehappo": "npm i --no-save happo-plugin-puppeteer",
"happo": "happo"

?

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 1, 2018

Author Contributor

TIL! I'll make this change.

@@ -25,6 +25,8 @@
"test": "npm run build && npm run tests-only",
"storybook": "start-storybook -p 6006",
"storybook:css": "npm run build:css && start-storybook -p 6006 -c .storybook-css",
"happo": "npm i --no-save happo-plugin-puppeteer && happo",
"happo-ci": "happo-ci-travis",

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 30, 2018

Member

why is this a separate binary? what does it do differently?

This comment has been minimized.

Copy link
@trotzig

trotzig Oct 1, 2018

Author Contributor

The first command will create a single report (set of screenshots) and can be run locally as well as in CI. The second command is designed to run in CI. It will (if needed) check out the commit that the PR is based on (usually a commit on master), run the first command, then check out the tip of the PR, run the first command again. As a final step, it will compare the two reports which will create a diff between the two commits.

@ljharb

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

The github app's been activated for this repo.

trotzig added a commit to trotzig/react-dates that referenced this pull request Oct 1, 2018

DRY up happo script invocation in .travis.yml
As suggested by @ljharb [1] in code review. The length of the command
and the duplication was definitely an issue. Making it shorter helps
readability and makes it more robust.

[1] airbnb#1382 (comment)

@trotzig trotzig force-pushed the trotzig:happo branch 2 times, most recently from e2ca710 to b2d6614 Oct 1, 2018

trotzig added a commit to trotzig/react-dates that referenced this pull request Oct 1, 2018

DRY up happo script invocation in .travis.yml
As suggested by @ljharb [1] in code review. The length of the command
and the duplication was definitely an issue. Making it shorter helps
readability and makes it more robust.

[1] airbnb#1382 (comment)

@trotzig trotzig force-pushed the trotzig:happo branch from b2d6614 to 99492ac Oct 1, 2018

trotzig added a commit to trotzig/react-dates that referenced this pull request Oct 1, 2018

DRY up happo script invocation in .travis.yml
As suggested by @ljharb [1] in code review. The length of the command
and the duplication was definitely an issue. Making it shorter helps
readability and makes it more robust.

[1] airbnb#1382 (comment)

@trotzig trotzig force-pushed the trotzig:happo branch from 99492ac to 11f1d50 Oct 1, 2018

trotzig added a commit to trotzig/react-dates that referenced this pull request Oct 1, 2018

DRY up happo script invocation in .travis.yml
As suggested by @ljharb [1] in code review. The length of the command
and the duplication was definitely an issue. Making it shorter helps
readability and makes it more robust.

[1] airbnb#1382 (comment)

@trotzig trotzig force-pushed the trotzig:happo branch 6 times, most recently from a4c0ee0 to 3586d25 Oct 1, 2018

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

Alright, I'm at a place now where I'm ready for a review. Sorry for all the back-and-forth and force-pushes, I had to do some trial-and-error to get a few things to work. While doing so, I found a bug in the happo.io client (first-time run didn't work as expected). I've also noted down a few things I could improve on:

To view the full list of screenshots for this PR, go to https://happo.io/a/32/compare/6860b219f7f3b3d588e9745e470baf0afd17e4e5/3586d25b47af481e71f18375f9b9cc9b7d82352c?t=added

@lencioni
Copy link
Member

left a comment

@trotzig can you resolve the merge conflict and update this PR?

@trotzig trotzig force-pushed the trotzig:happo branch from 3586d25 to 9ef25f9 Jun 4, 2019

@coveralls

This comment has been minimized.

Copy link

commented Jun 4, 2019

Coverage Status

Coverage remained the same at 84.14% when pulling 1561183 on trotzig:happo into 11de201 on airbnb:master.

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@lencioni working on it!

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

It looks like the regular .storybook setup can't be built statically. Running this on the master branch currently fails:

$(npm bin)/build-storybook

I can't make much of the error message:

ERR! ./examples/DateRangePickerWrapper.jsx
ERR! Module build failed (from ./node_modules/babel-loader/lib/index.js):
ERR! TypeError: Property declarations[0] of VariableDeclaration expected node to be of a type ["VariableDeclarator"] but instead got "ExpressionStatement"
ERR!     at validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:128:13)
ERR!     at validator (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:97:7)
ERR!     at Object.validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:172:7)
ERR!     at validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/validators/validate.js:17:9)
ERR!     at builder (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/builders/builder.js:46:27)
ERR!     at Object.VariableDeclaration (/home/travis/build/airbnb/react-

(full log here: https://travis-ci.org/airbnb/react-dates/jobs/541444935)

It looks like others are running into this as well: #1595 (comment)

@lencioni any chance you can look into that?

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I'd expect happo's webpack build to run in NODE_ENV development, not production, which i would expect not to include minification.

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@ljharb It does, but the happo-plugin-storybook plugin avoids the happo webpack build and uses a static storybook build instead.

I've got things working with Storybook version 5. Is there anything you see stopping us from getting on to the latest storybook version? If not I'll push a separate PR with that change.

trotzig added a commit to trotzig/react-dates that referenced this pull request Jun 5, 2019

Update to Storybook v5
I was running into issues making a static Storybook build on v4 of
Storybook (see airbnb#1382 (comment)).

The error I'm seeing is something like

ERR! ./examples/DateRangePickerWrapper.jsx
ERR! Module build failed (from ./node_modules/babel-loader/lib/index.js):
ERR! TypeError: Property declarations[0] of VariableDeclaration expected node to be of a type ["VariableDeclarator"] but instead got "ExpressionStatement"
ERR!     at validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:128:13)
ERR!     at validator (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:97:7)
ERR!     at Object.validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/definitions/utils.js:172:7)
ERR!     at validate (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/validators/validate.js:17:9)
ERR!     at builder (/home/travis/build/airbnb/react-dates/node_modules/@babel/types/lib/builders/builder.js:46:27)
ERR!     at Object.VariableDeclaration (/home/travis/build/airbnb/react-

It seems to happen at the minification step, because right before that
error happens I'm seeing "92% chunk asset TerserPlugin".

I started going down the route of tracing this down in Storybook v4, but
then decided to give v5 a spin instead. It turns out the upgrade isn't
all that complicated, and it does resolve the error I was seeing.

The only change needed except for bumping all the @storybook
dependencies was to turn the webpack config from "extend mode" to "full
control mode". See https://storybook.js.org/docs/configurations/custom-webpack-config/

I used this guide as a starting point to the upgrade:
https://medium.com/storybookjs/storybook-5-migration-guide-d804b38c739d

@trotzig trotzig referenced this pull request Jun 5, 2019

Merged

Update to Storybook v5 #1673

@ljharb

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

As long as it works, and doesn't force any webpack or babel or react versions (like if it makes npm ls error out on react < 16, for example, then we couldn't test on that react version properly), then a PR sounds great!

trotzig added some commits Jun 19, 2018

Reuse webpack configs across storybook projects
I noticed that the webpack config files were the same for storybook and
storybook-css. Instead of duplicating content, we can just inherit one
from the other.
Integrate with Happo.io
Happo.io is a screenshot testing service. It is used to find bugs,
visualize change and provide a visual log of your application's
components. It can be used to take screenshots in different browsers.

I've been using react-dates for a while when stress-testing happo. One
of the things that have been most tricky is that react-dates relies on
measuring elements in the DOM as part of rendering the UI. This is an
issue for default Happo, because it uses JSDOM to prerender components,
then collects the HTML and CSS and sends it to the happo.io to take
screenshots. JSDOM can't measure things. The pre-rendering phase is
important for speed and security, and is something that would be less
than trivial to bypass.  So, instead of bypassing pre-rendering I've
made it pre-render in a real browser. The `happo-plugin-puppeteer` npm
module injects a puppeteer-powered (uses Chrome) pre-renderer to Happo.
And by using that module, we're good to go for react-dates.

Another important module here is `happo-plugin-storybook`. This plugin
converts all storybook stories to happo examples. This means we can keep
using Storybook and we'll get screenshot testing "for free". I picked
the .storybook-css project and disabled a few decorators/addons in the
storybook config.

For now, we'll be taking screenshots in Chrome in two viewports:

- 900x600 ("desktop")
- 320x640 ("mobile")

Going forward, we may choose to add other targets/browsers as well. But
to begin with, I want to make sure things are working smoothly before we
expand.
Move happo-plugin-puppeteer to optionalDependencies
Installing puppeteer (which is a dependency of happo-plugin-puppeteer)
fails on Node 4.

https://travis-ci.org/airbnb/react-dates/jobs/435084749

Neither happo nor puppeteer supports Node 4, and I intend to only run
happo in Node 8 in this project. So moving it to an optional dependency
should be okay.
Add happo run to Travis config
This commit will make sure that the happo-ci-travis script (provided by
the happo.io module) is invoked during a Travis job. We're only
interested in running happo on one version of Node, and one version of
React, hence the if-statement for the happo script in .travis.yml
Move happo-plugin-puppeteer back to devDependencies
@ljharb pointed out to me that this would make happo-plugin-puppeteer a
regular dependency shipped with the library, which is not what I want. I
have a different solution for the puppeteer installation error (which is
what caused me to move it to peer in the first place). I'll explore that
solution in upcoming commits.
Skip the install phase of the puppeteer dependency
Puppeteer is a dependency of happo-plugin-puppeteer. It is causing
installation errors on Node 4. By passing the
PUPPETEER_SKIP_CHROMIUM_DOWNLOAD flag I believe we can avoid the error.

I found out about this env flag in the Troubleshooting section in the
puppeteer docs:
https://github.com/GoogleChrome/puppeteer/blob/395c50624cdd82efa5516201d587b7e578944acf/docs/troubleshooting.md#troubleshooting

I still have to figure out how to get it installed. But this commit will
at least prevent the installation failure.
Install happo-plugin-puppeteer only when it's needed
Installing puppeteer (a dependency of happo-plugin-puppeteer) has proven
hard on Node 4. See for instance this Travis error log:

  > puppeteer@1.8.0 install /home/travis/build/airbnb/react-dates/node_modules/puppeteer
  > node install.js
  /home/travis/build/airbnb/react-dates/node_modules/puppeteer/install.js:74
    return Promise.all([...cleanupOldVersions, generateProtocolTypesIfNecessary(true /* updated */)]);
                        ^^^
  SyntaxError: Unexpected token ...

From https://travis-ci.org/airbnb/react-dates/jobs/435318847

Since we don't use happo in Node 4, we might as well delay installation
until we need the plugin, which is right before we run `happo`.

The downside of this approach is that running happo locally will be a
little slower, since the installation step is always run ahead of
invoking happo. And people might be confused about seeing a reference to
a package not listed in package.json.

This commit also includes a simplification to how the happo test script
is invoked by Travis. I had misunderstood how the matrix worked.
Add Happo API tokens as secrets to .travis.yml
To properly run happo for the project, we need to provide it with API
tokens. At first, I thought that an owner of the repo had to add these,
but if I understand things correctly, anyone is able to add keys.

https://docs.travis-ci.com/user/encryption-keys/

These keys will make Happo runs work for anyone with access to the repo.
They will not be available to forks/outside PRs (for security reasons).
Happo relies on a different auth mechanism for outside PRs, and for that
to work we need to install the Happo Github app. There are some
limitations to that auth mechanism however, and having the keys here for
members of the repo will help make things smoother.
Introduce prehappo script
@ljharb taught me a new trick with npm scripts. Instead of inlining the
preparation step for the happo script, we can add a "prehappo" script.

From the npm docs:

  Additionally, arbitrary scripts can be executed by running npm
  run-script <stage>. Pre and post commands with matching names will be
  run for those as well (e.g. premyscript, myscript, postmyscript).
  Scripts from dependencies can be run with npm explore <pkg> -- npm run
  <stage>.

https://docs.npmjs.com/misc/scripts
Explain secure variables in .travis.yml
These env secrets were added as part of setting up the integration with
happo.io. They are read in `.happo.js` and passed along to happo.io as
part of running `npm run happo`.

For anyone ending up here wondering how these got here, or how you can
generate new ones, here's the process I went through:

1. First, grab the API tokens from the account page for the react-dates
project over at happo.io: https://happo.io/a/32/account . You need to be
an administrator of the account to do this - send an email to
info@happo.io in case you can't get access.

2. Use the travis gem to generate the secure tokens:

   gem install travis
   travis encrypt HAPPO_API_KEY=<key>
   travis encrypt HAPPO_API_SECRET=<secret>

Where <key> and <secret> are the tokens you found over at happo.io.

3. Replace/add the secure tokens to .travis.yml
Bump happo.io to 3.0.0-rc.3
This version has a bugfix for how we check for the happo.io package
being installed.
Run `build:css` before running happo
I'm getting this error in Travis right now:

  ModuleNotFoundError: Module not found: Error: Can't
  resolve '../css/styles.css' in
  '/home/travis/build/airbnb/react-dates/.storybook-css' at
  factoryCallback
  (/home/travis/build/airbnb/react-dates/node_modules/webpack/lib/Compilation.js:264:39)

Running build:css before running happo will make sure the file is always
there.
Make puppeteer work in Travis
From the troubleshooting docs for puppeteer:

  To run headless Chrome on Travis, you must call launch() with flags to
  disable Chrome's sandbox, like so:

  const browser = await puppeteer.launch({args: ['--no-sandbox']});

https://github.com/GoogleChrome/puppeteer/blob/master/docs/troubleshooting.md#running-puppeteer-on-travis-ci
Bump happo.io to 3.0.0
This version has a bugfix for the first happo run in a repository.
Instead of failing with a 404 in one of the last steps, it will succeed
and generate a diff with only added snapshots.
Update happo-plugin-storybook
A lot has happened since I first added this plugin. First of all, we no
longer need happo-plugin-puppeteer, the latest storybook plugin will
render stories remotely on happo.io workers. So pre-rendering in chrome
has no effect.

I also switched from using the .storybook-css project to regular
.storybook. I can't recall exactly why I did this to begin with, but the
regular one seems to work well now at least.

Docs for happo-plugin-storybook (including how we inject it into our
storybook setup) are available here:
https://github.com/happo/happo-plugin-storybook
Remove "Show info" button from Happo snapshots
The info addon is showing a blue button at the top for all stories. We
don't want that to end up in the happo screenshots. I tried to find a
more elegant way than wrapping the `withInfo` function, but in the end
the wrapper made most sense. There is of course a risk people will
forget to use the wrapper when they add new stories, but that wouldn't
be the end of the world.
Add a 200ms delay for Happo
While testing the happo run, I've noticed that a number of story
screenshots are cut off. Happo is quite eager at taking the screenshot,
and I think what's happening is that the component isn't fully done
rendering at the time the screenshot is taken.

Using a delay isn't ideal as A) there is still a risk for cutoff
screenshots, and B) it increases the total run time by 200ms times the
number of stories. In a mature open source project like this, I don't
expect the test suite to be run more than perhaps a few times per day
however, so the performance thing might not be terrible after all. I'll
continue investigating whether we can do better. One idea I had was that
stories themselves can call a function when they're done. Something like

  import { done } from 'happo-plugin-storybook/register';

  storiesOf('DayPicker', module)
    .add('default', withInfo()(() => (
      <DayPicker onDone={done} />
    )), { happo: { usesCallback: true } })

I don't yet know how to implement that behavior though, both in the
component itself and on the happo-plugin-storybook side.
Update happo.io client to 3.17.0
A lot has happened since 3.0.0 (the previous version), but for our usage
I think we won't notice any differences.
Add core-js@3 as a dev dependency
After installing happo.io in this project after the upgrade to Storybook
v5, I started running in to problems with the wrong core-js version
being used. @storybook/core is depending on core-js@3 but the version
hoisted at the root of node_modules was v2. Which should be fine, any
@storybook/core code should pick the local version
(the one found under node_modules/@storybook/core/node_modules). But
that didn't seem to work in all cases, and I saw things like

  ERROR in ./.storybook/config.js
  Module not found: Error: Can't resolve 'core-js/modules/es.array.for-each' in '/indio-ui/.storybook'
   @ ./.storybook/config.js 1:0-43
   @ multi ./node_modules/@storybook/core/dist/server/common/polyfills.js ./node_modules/@storybook/core/dist/server/preview/globals.js ./.storybook/config.js ./node_modules/webpack-hot-middleware/client.js?reload=true

The happo.io library depends on core-js@2, and it was after installing
happo.io that v2 started getting hoisted into the root. It seems
Storybook is always using the root-most version, not sure why. This
issue hints at many others running into the same issue, or something
similar:
storybookjs/storybook#6204

There are many copies of core-js in our dependency tree. `karma` depends
on core-js@2, `react-addons-shallow-compare` depends on core-js@1 (via
`fbjs`).

Adding a direct dependency on core-js resolves the issues. Once there is
a proper solution for
storybookjs/storybook#6204, we can most likely
remove this dependency.

@trotzig trotzig force-pushed the trotzig:happo branch from 84e1c5a to 1561183 Jun 6, 2019

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Sorry for all the force-pushing. The last one was rebasing on master again after the Storybook v5 upgrade was merged.

I also had to resolve an issue with Storybook v5 and its usage of core-js. For anyone reviewing, it might be worth taking a look at 1561183 in isolation (lengthy commit message) and the Storybook issue that's related to the fix.

@trotzig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@lencioni did you try accepting the happo diff as well. You should (theoretically) have access, as long as you have write-access to the github repo (react-dates).

@lencioni

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

@trotzig I did, but it didn't seem to change the status on github. It looks like when I select Accept, I get a 401 response from the API request, but the UI doesn't indicate any problems.

@@ -70,6 +72,7 @@
"babel-preset-airbnb": "^3.2.1",
"chai": "^4.2.0",
"clean-css": "^4.2.1",
"core-js": "^3.1.3",

This comment has been minimized.

Copy link
@ljharb

ljharb Jun 9, 2019

Member

why is this needed?

This comment has been minimized.

Copy link
@trotzig

trotzig Jun 9, 2019

Author Contributor

The commit message has the details: 1561183

This comment has been minimized.

Copy link
@ljharb

ljharb Jun 9, 2019

Member

The implication is that by using storybooks webpack config we have created this problem for ourselves. should that be reverted?

This comment has been minimized.

Copy link
@trotzig

trotzig Jun 9, 2019

Author Contributor

I think there’s no real way out of using storybook’s webpack config. And just to be clear, this isn’t a happo issue - it just happened to trigger the issue.

Perhaps I’m misunderstanding something though, can you elaborate on what you mean should be reverted?

This comment has been minimized.

Copy link
@ljharb

ljharb Jun 9, 2019

Member

why must we use their config, versus manually replicating the parts we need (since the linked issue is due to weird aliasing in it)?

This comment has been minimized.

Copy link
@trotzig

trotzig Jun 9, 2019

Author Contributor

The alias isn’t there anymore, tried removing it. A theory I have is that they’re generating some code that ends up being bundled with webpack. At least that would explain why a require('core-js') wouldn’t resolve to the local copy of core-js (storybook/core/node_modules/core-js). If I have some time I’ll explore this theory and try to push a fix upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.