Storybook: Storyshots ships *.ts files in dist/ directory

[bard]

Describe the bug

The current (beta.37 and beta.38) Storyshots package contains TypeScript files along with transpiled files in dist/. This causes jest to (try to) import the wrong file.

To Reproduce

1. git clone https://github.com/bard/repro-storyshots-dist-ts
2. cd repro-storyshots-dist-ts
3. yarn
4. yarn jest storyshots.test.ts

    /tmp/project/node_modules/@storybook/addon-storyshots/dist/frameworks/frameworkLoader.ts:2
    import fs from 'fs';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

    > 1 | import initStoryshots from '@storybook/addon-storyshots';
        | ^
      2 | 
      3 | initStoryshots()
      4 | 

Expected behavior

Storyshots gets imported without error.

System:

Environment Info:

  System:
    OS: Linux 5.7 Arch Linux
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
  Binaries:
    Node: 14.4.0 - /usr/sbin/node
    Yarn: 1.22.4 - /usr/sbin/yarn
    npm: 6.14.5 - /usr/sbin/npm
  Browsers:
    Firefox: 77.0.1
  npmPackages:
    @storybook/addon-storyshots: 6.0.0-beta.38 => 6.0.0-beta.38 

[shilman]

@bard any idea when this change was introduced?

[bard]

It looks like it was introduced in -beta.28. Up to -beta.27, only javascript plus declaration files can be found under dist/frameworks.

[bard]

No longer happening with current version. Closing.

[EdenTurgeman]

@bard @shilman Currently happening for me with 6.0.0-rc.3

storybook git:(SG-200-Storybook-upgrade) ✗ jest
 FAIL  ./imageSnapshots.test.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    /Users/edenturgeman/*___*/*___*/storybook/node_modules/@storybook/addon-storyshots/dist/frameworks/frameworkLoader.ts:2
    import fs from 'fs';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

      at Runtime.createScriptFromCode (../node_modules/jest-runtime/build/index.js:1059:14)
      at Object.<anonymous> (node_modules/@storybook/addon-storyshots/dist/api/index.js:41:41)

Seems like it's a typescript problem, and it's been happening since upgrading to beta.30±

[bard]

@EdenTurgeman is correct, I was mistakenly looking at @storybook/addon-storyshots 5.x.

[EdenTurgeman]

Unfortunately Still happening in the latest RC (12)

[EdenTurgeman]

@Yama-Tomo Did you manage to solve this? if so, how and why?

[floAr]

Also having this issue after doing the zero step config and adding storyshots @ next. Any workaround or eta for a fix?

[bard]

I looked into this about a month ago and it appeared that the change was introduced between -beta.27 and -beta.28, verified naively by installing one version after another until the typescript files showed up.

I reviewed the diff between the two tags and couldn't immediately see the cause.

If anyone has the bandwidth, best thing to help @shilman and team would be to git-bisect between the two tags, and narrow the problem down to the actual commit. It's a simple but (as it involves rebuilding) probably time-consuming process.

[Yama-Tomo]

@EdenTurgeman
Hi ✋
First, I'm using typescript, ts-jest and @storybook/vue.
https://github.com/Yama-Tomo/vue-vuex-typescript-sample/blob/cff9e6da36dcb32e1576275bfebb771101bf9a4b/package.json

My workaround is as follows

• Make the ts files under the node_modules/@storybook/addon-storyshots/dist/* can be transpiled by ts-jest.
  
  
  
  • If this package was provided js files only, you wouldn't need it, but since we have ts files, ts-jest will try to import the dependent ts files when we transpile the test code. So you need to be able to transpile the files.
  
  
• (optional) Configuration to ignore type errors under node_modules.

See also this commits: https://github.com/Yama-Tomo/vue-vuex-typescript-sample/commit/b83262757f70f253ef0ddd6bf94f00396d9f8144

[floAr]

@bard & @shilman I took the time to do a full bisect and it seems that 94a969a246ea6fd9624f21d9ad95c309e5612586 is the culprit. I cant really tell what the problem is there due to my limited knowledge of the whole build ecosystem. Hope this helps a bit!

[floAr]

Digging deeper, I found the actual error: In cd61e43e8ba71821caf089e3efb2a688003e31e4 there is a change to exclude framework folder from cleanup for the cli in the prepare script. This seems to have the side-effect on the framework folder withing storyshots. I will see if I can muster the regex knowledge to fix this :)

[shilman]

Awesome detective work @floAr. Hero! 🥇

cc @tooppaaa

[floAr]

Added a simple fix, that I was able to validate on my machine.

Problem was that we cant change the regex (/dist\/frameworks\/.*/.test(filePath) as it operates only relative to the dist/ directory. So there is no way to check if we are in cli/dist or storyshot/dist.
This fix tests initially if we are in the 'storyshot' directory and in that case disregards the regex.

This means we keep the current behaviour (ts files in /frameworks/ folders are not cleaned up) except for the storyshots dist. Alternative would be to flip it around to: always clean up ts files except for the cli folder
@shilman what do you think makes more sense?

[shilman]

cc @tooppaaa @gaetanmaisse @yannbf ☝️

[tooppaaa]

(sorry I'm slow to reply...)

I think this is an excellent idea. Now that we introduced a framework folder, the logic to cleanup folders is more complex when it should be simple.

On the other hand, it took some time to see the issue and fix it, I'm not sure this should be done now. short time after 6.0 ?

[shilman]

Zoinks!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-rc.30 containing PR #11792 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.