happo / happo-plugin-storybook

A happo.io plugin for Storybook
MIT License
11 stars 5 forks source link

`isHappoRun` returns true even when happo is not run, i.e. when opening the iframe view directly #17

Closed jack-sf closed 5 years ago

jack-sf commented 5 years ago

We often open our stories directly (like localhost:9000/iframe.html?... instead of localhost:9000/?...), to have the full window preview of the story. (i.e. to open it in browserstack or for better developer debugging experience).

Unfortunately, when opening it in such a way, isHappoRun returns true ( https://github.com/happo/happo-plugin-storybook/blob/d1431b35b4fc234fb89ff0be67f85c087392c3b5/register.es6.js#L97 ).

This incorrectly makes our stories to be run in happo mode, when we don't want.

To fix it, isHappoRun maybe should be some function that checks for environmental variable that is appended only when the app is actually run in the happo mode? (just thinking out loud)

jack-sf commented 5 years ago

For now we wrote our custom function isHappoRun and use it instead, but it's way less than perfect...:

// isHappoRun will always return true, unless HAPPO_MODE is explicitly defined to false
// (Workaround for: https://github.com/happo/happo-plugin-storybook/issues/17)
export function isHappoRun() {
  const happoDisabled = [false, 'false', 0, '0'].includes(
    process.env.HAPPO_MODE,
  );
  return !happoDisabled;
}

Our package json now looks like this (we had to prepend HAPPO_MODE=false to all our non-happo commands):


  "scripts": {
    "storybook:serve": "HAPPO_MODE=false start-storybook -p 9001 -c storybook",
    "storybook:build": "HAPPO_MODE=false build-storybook -c storybook -o storybook-dist",
    "happo": "happo",
    "happo-ci-pr": "happo-ci-circleci",
  },

It would be way easier if all happo scripts would define some global/env variable, so we could easily detect if we're in happo mode basing on that env variable.

trotzig commented 5 years ago

Yeah, this isn’t ideal. I think one solution would be to inject some (unique) global variable in the iframe.html produced by happo. We already modify the html a little, and we might as well throw in a global js variable: https://github.com/happo/happo-plugin-storybook/blob/master/index.js#L63

jack-sf commented 5 years ago

Yeah, that would be a good solution probably as well.

trotzig commented 5 years ago

A fix for this issue was included in the 2.1.3 release. https://github.com/happo/happo-plugin-storybook/releases/tag/v2.1.3