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

Fix: reacts urlForAssets loading non js,css files #1554

Merged
merged 9 commits into from
Aug 18, 2017
Merged

Fix: reacts urlForAssets loading non js,css files #1554

merged 9 commits into from
Aug 18, 2017

Conversation

ianmcnally
Copy link
Contributor

Issue: when building a non css or js file with webpack, storybook would throw the error:

TypeError: Cannot read property 'push' of undefined
    at /Users/me/project/node_modules/@storybook/react/dist/server/iframe.html.js:69:33
    at Array.forEach (native)

What I did

I added a webpack plugin to build an external svg.

That produced an assets bundle:

assets { preview:
   [ 'static/preview.bundle.js',
     'design-system-components.css',
     'static/preview.bundle.js.map',
     'design-system-components.css.map' ],
  manager: [ 'static/manager.bundle.js', 'static/manager.bundle.js.map' ],
  'patterns-sprite.svg': 'patterns-sprite.svg' }

Which the logic in iframe.html.js fails to consider any extension beyond what it's define (js, css).

How to test

Include a non css or non js file in your webpack bundle. Or you could rename something generated by extract text webpack plugin to something non js or css.

@codecov
Copy link

codecov bot commented Aug 1, 2017

Codecov Report

Merging #1554 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1554      +/-   ##
=========================================
+ Coverage   20.82%   20.9%   +0.07%     
=========================================
  Files         251     251              
  Lines        5579    5583       +4     
  Branches      669     678       +9     
=========================================
+ Hits         1162    1167       +5     
+ Misses       3903    3900       -3     
- Partials      514     516       +2
Impacted Files Coverage Δ
app/vue/src/server/iframe.html.js 0% <ø> (ø) ⬆️
app/react/src/server/iframe.html.js 73.91% <100%> (+10.75%) ⬆️
app/react/src/server/utils.js 0% <0%> (-32.15%) ⬇️
addons/info/src/components/markdown/htags.js 30% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.86% <0%> (ø) ⬆️
app/react-native/src/preview/story_kind.js 0% <0%> (ø) ⬆️
addons/knobs/src/KnobManager.js 32% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
addons/info/src/components/Props.js 37.2% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/menu_item.js 19.14% <0%> (ø) ⬆️
... and 23 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 45ec15e...3ad18e8. Read the comment docs.

@ndelangen
Copy link
Member

Does this change not also affect the vue app?

@ianmcnally
Copy link
Contributor Author

ianmcnally commented Aug 3, 2017

@ndelangen

Does this change not also affect the vue app?

The issue is present in the Vue app too, having just looked it it. Looks like the same code in app/vue/.../iframe.html.js.

As for how to proceed with this PR, would you like to see that change there, an approach to share the code, or just to get this into the react app?

@ndelangen
Copy link
Member

Copy the fix into the vue code sounds like the best course of action for now.

Yes I know this feels bad. It's a conscious choice to duplicate this code, and I knew there would be some pains because of it.

But we know where the duplication really hits us and with the angular work we'll know much better what we should abstract into common bits and what we shouldn't.

@ianmcnally
Copy link
Contributor Author

ianmcnally commented Aug 14, 2017

Totally understand @ndelangen. Added the code in the view app in 6908619.

@satazor
Copy link
Contributor

satazor commented Aug 18, 2017

Just wanted to let you guys know that I also have hit the same issue. Is there anything missing for this MR to land?

@ianmcnally is there any quick workaround for this issue?

@ndelangen
Copy link
Member

Verified by adding a svg-file as additional entrypoint in a custom webpack config.

@ndelangen ndelangen merged commit 7ea6c54 into storybookjs:master Aug 18, 2017
@ndelangen ndelangen self-assigned this Aug 18, 2017
@ianmcnally ianmcnally deleted the react-fix-url-for-assets-loading branch August 18, 2017 18:29
@ianmcnally
Copy link
Contributor Author

@satazor you could point to the commit into master 7ea6c54

@satazor
Copy link
Contributor

satazor commented Aug 21, 2017

@ianmcnally I installed from git pointing to that commit, but I can't specify the subdirectory (app/react). Any alternative way to install?

"@storybook/react": "git+ssh://git@github.com:storybooks/storybook.git#7e60bc074696f2a7c3f66db18a5855d1dfa0f587",

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 21, 2017

You can specify it when importing: import {...} from '@storybook/react/app/react'

@satazor
Copy link
Contributor

satazor commented Aug 21, 2017

The issue is that when installing from git:

  • start-storybook bin is not installed
  • node_modules/@storybook/react is not present
  • node_modules/storybook is not present either

Here's my package.json devDependencies:

    "@storybook/addon-actions": "^3.2.0",
    "@storybook/addon-info": "^3.2.4",
    "@storybook/addon-knobs": "^3.2.0",
    "@storybook/addon-storyshots": "^3.2.3",
    "@storybook/react": "storybooks/storybook#7e60bc074696f2a7c3f66db18a5855d1dfa0f587",

I'm using npm5. Using npm4, the package installs but start-storybook is unavailable.
Alternatively, perhaps we could release this feature?

Thanks.

@ndelangen
Copy link
Member

@ianmcnally I installed from git pointing to that commit, but I can't specify the subdirectory (app/react). Any alternative way to install?

Indeed this is not possible. This feature has been requested but npm has shown 0 interest in adding it.

@ndelangen
Copy link
Member

We will release to npm soon!

@ndelangen
Copy link
Member

Should be on npm!

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

Successfully merging this pull request may close these issues.

6 participants