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 @storybook/client-logger import #2576

Merged
merged 3 commits into from
Dec 27, 2017
Merged

Fix @storybook/client-logger import #2576

merged 3 commits into from
Dec 27, 2017

Conversation

eur00t
Copy link

@eur00t eur00t commented Dec 27, 2017

Issue:
@storybook/client-logger was imported incorrectly in @storybook/addon-info, which results in runtime error TypeError: Cannot read property 'warn' of undefined

What I did

Replaced default import with named import.

How to test

  1. Use marksyConf in storybook configuration.
  2. When running storybook in the browser, proper warning message should be shown instead of the runtime error.

Is this testable with jest or storyshots? No

Does this need a new example in the kitchen sink apps? No

Does this need an update to the documentation? No

If your answer is yes to any of these, please make sure to include it in your PR.

@Hypnosphi Hypnosphi added addon: info bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Dec 27, 2017
@codecov
Copy link

codecov bot commented Dec 27, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2576   +/-   ##
=======================================
  Coverage   32.68%   32.68%           
=======================================
  Files         398      398           
  Lines        8838     8838           
  Branches      952      959    +7     
=======================================
  Hits         2889     2889           
+ Misses       5286     5278    -8     
- Partials      663      671    +8
Impacted Files Coverage Δ
addons/info/src/index.js 78.57% <ø> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/react/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/a11y/src/components/Report/Info.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
addons/knobs/src/components/types/Color.js 8.21% <0%> (ø) ⬆️
addons/actions/src/lib/decycle.js 53.52% <0%> (ø) ⬆️
... and 59 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 7781c54...1b1c38e. Read the comment docs.

@Hypnosphi
Copy link
Member

Hypnosphi commented Dec 27, 2017

Nice catch @eur00t! I wonder why eslint didn’t catch it though

Upd: it turned out that we just don't use the corresponding imports/default rule. I'll add that (and probably some others)

@eur00t
Copy link
Author

eur00t commented Dec 27, 2017

@Hypnosphi thanks! I think it'll be helpful to add import/default and import/named eslint rules from eslint-plugin-import (I can see that currently we use import/no-unresolved only).
With import/default there can be problems though, because some packages doesn't export ES6 modules, for example @storybook/client-logger. It might work when "jsnext:main" is defined.

@Hypnosphi Hypnosphi merged commit fd2b0fc into storybookjs:master Dec 27, 2017
@Hypnosphi Hypnosphi removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Dec 28, 2017
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.

2 participants