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

Remove src/ from remaining .npmignores. #2678

Merged
merged 2 commits into from
Jan 8, 2018
Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jan 8, 2018

Following on from #2677

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2678   +/-   ##
=======================================
  Coverage   34.35%   34.35%           
=======================================
  Files         389      389           
  Lines        8747     8747           
  Branches      910      910           
=======================================
  Hits         3005     3005           
  Misses       5121     5121           
  Partials      621      621

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 5664113...61e2e38. Read the comment docs.

@shilman shilman merged commit c6a841e into master Jan 8, 2018
@shilman shilman deleted the tmeasday/update-npmignores branch January 8, 2018 04:26
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 8, 2018
shilman added a commit that referenced this pull request Jan 8, 2018
Remove `src/` from remaining `.npmignore`s.
@joscha
Copy link
Member

joscha commented Jan 10, 2018

This change breaks storybook in 3.3.6 via:


info @storybook/react v3.3.6
--
  | info
  | info => Loading custom addons config.
  | info => Loading custom webpack config (extending mode).
  | info Building storybook ...
  | ERR! Failed to build the storybook
  | ERR! ./node_modules/@storybook/addon-knobs/src/react/index.js
  | ERR! Module parse failed: Unexpected token (25:9)
  | ERR! You may need an appropriate loader to handle this file type.
  | ERR! \|   const initialContent = getStory(context);
  | ERR! \|   const props = { context, storyFn: getStory, channel, knobStore, initialContent };
  | ERR! \|   return <WrapStory {...props} />;
  | ERR! \| };
  | ERR! \|
  | ERR!  @ ./node_modules/@storybook/addon-knobs/dist/register.js 3:13-29
  | ERR!  @ ./node_modules/@storybook/addon-knobs/register.js
  | ERR!  @ ./conf/storybook/addons.js
  | ERR!  @ multi ./node_modules/@storybook/react/dist/server/config/polyfills.js ./conf/storybook/addons.js ./node_modules/@storybook/react/dist/client/manager

because presumably now "jsnext:main": "src/index.js", is picked up, which expects a webpack env that can load an es6-file (which is not true for all projects, for example ours doesn't have any es6, but only TS)

@joscha
Copy link
Member

joscha commented Jan 10, 2018

removing src fixes it, not so sure about the jsnext:main part, yetm continuing to investigate.

@joscha
Copy link
Member

joscha commented Jan 10, 2018

I am not sure it's related to jsnext:main, as we import '@storybook/addon-knobs/register'; in addons.js. I am just not following, how we get from the chain addons.js -> addon-knobs/register.js -> addon-knobs/dist/register.js suddenly to addon-knobs/src/react/index.js

@tmeasday
Copy link
Member Author

tmeasday commented Jan 12, 2018

@joscha let's take it to the other issue but it really seems like the require('react') line (in addon-knobs/dist/register.js 3:13-29) is somehow requiring addon-knobs/src/react/index.js

@joscha
Copy link
Member

joscha commented Jan 12, 2018

@joscha let's take it to the other issue but it really seems like the require('react') line (in addon-knobs/dist/register.js 3:13-29) is somehow requiring addon-knobs/src/react/index.js

yes, I can't make sense of that, though.

@Hypnosphi
Copy link
Member

I can't make sense of that

@joscha please share your .storybook/webpack.config.js file in #2704 and we'll try to figure it out together

@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants