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

Add missing regenerator and runtime babel transform pkgs to package.json #1848

Merged
merged 3 commits into from
Sep 16, 2017

Conversation

valscion
Copy link
Contributor

app/react/src/server/config/babel.js uses require.resolve on both of
these packages, yet as they are not in the dependencies, they won't be
found unless they are installed alongside storybook itself

Issue: fix #1729

What I did

I added the missing packages to package.json

How to test

See https://github.com/valscion/storybook-repro-1729/tree/master for the broken case. Run npm install with npm@2.x version to force an error from these missing dependencies.

Note that this is not a specific issue for npm@2.x — it could happen also should newer npm versions not install these packages at root level for one reason or another.

A fixed case with this change is here: https://github.com/valscion/storybook-repro-1729/tree/fix-repro

app/react/src/server/config/babel.js uses require.resolve on both of
these packages, yet as they are not in the dependencies, they won't be
found unless they are installed alongside storybook itself
@valscion
Copy link
Contributor Author

Now, as I was working on the reproduction repository, I also noticed that a similar issue can be found with babel-runtime: valscion/storybook-repro-1729@ce32cac

I'm not really sure how we could test for these missing dependencies in CI, given this monorepo architecture 😕 . If anyone has any ideas on how that could work, I could try to work on a proof of concept. These subtle dependency errors are very nasty, and due to how yarn & npm@3 and forwards lifts all dependencies to the top level, they aren't easily discoverable either 😰

@codecov
Copy link

codecov bot commented Sep 15, 2017

Codecov Report

Merging #1848 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1848   +/-   ##
=======================================
  Coverage   21.33%   21.33%           
=======================================
  Files         257      257           
  Lines        5737     5737           
  Branches      689      685    -4     
=======================================
  Hits         1224     1224           
+ Misses       3989     3986    -3     
- Partials      524      527    +3
Impacted Files Coverage Δ
app/react/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/react/src/server/babel_config.js 0% <0%> (-77.42%) ⬇️
lib/ui/src/modules/ui/components/menu_item.js 19.14% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 8.51% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 12% <0%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05edc3e...9388fbb. Read the comment docs.

@Hypnosphi
Copy link
Member

Hypnosphi commented Sep 15, 2017

Please do the same for app-vue (app-react-native happens to have those deps already)

As for babel-runtime, looks like we should have it as peerDependency, because after the transform, the code from client app package will require it. We should also adjust CLI templates in that case. I think it should be done in a separate PR though, as it deserves further discussion

@valscion
Copy link
Contributor Author

Please do the same for app-vue (app-react-native happens to have those deps already)

Done! ☺️

As for babel-runtime, looks like we should have it as peerDependency, because after the transform, the code from client app package will require it. We should also adjust CLI templates in that case. I think it should be done in a separate PR though, as it deserves further discussion

Mmm yeah, another PR sounds reasonable. I don't think I'll be working on that, as it does sound a controversial change and I'm afraid I don't have enough insight information to lead that discussion in a fruitful fashion.

@Hypnosphi Hypnosphi mentioned this pull request Sep 16, 2017
@Hypnosphi Hypnosphi merged commit 386ce5a into storybookjs:master Sep 16, 2017
@valscion valscion deleted the fix-1729 branch September 17, 2017 14:59
@ndelangen
Copy link
Member

Awesome @valscion thank you !!

@glrodasz
Copy link

Could this change be added to the 3.3.0-alpha.0 version?

@valscion
Copy link
Contributor Author

@glrodasz, if you didn't notice, this change was shipped as part of v3.2.10

@StevenFewster
Copy link

I'm getting this error on 4.0.4, is it something that has been missed in the transition from 3 to 4, or am I experiencing something new and exciting?

It's happening for me in an Nx workspace with @angular/cli 7.

Error: Cannot find module '@babel/plugin-transform-runtime' from '\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli' at Function.module.exports [as sync] (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\resolve\lib\sync.js:43:15) at resolveStandardizedName (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\files\plugins.js:101:31) at resolvePlugin (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\files\plugins.js:54:10) at loadPlugin (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\files\plugins.js:62:20) at createDescriptor (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\config-descriptors.js:154:9) at items.map (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\config-descriptors.js:109:50) at Array.map (<anonymous>) at createDescriptors (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\config-descriptors.js:109:29) at createPluginDescriptors (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\config-descriptors.js:105:10) at plugins (\\SysProf\AppData$\redacted\AppData\Roaming\npm-cache\_npx\9860\node_modules\@storybook\cli\node_modules\@babel\core\lib\config\config-descriptors.js:40:19)

@valscion
Copy link
Contributor Author

@StevenFewster can you create a new issue for your case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants