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

Use babelrc: false only for custom config in .storybook directory #4077

Merged
merged 3 commits into from
Aug 27, 2018

Conversation

Hypnosphi
Copy link
Member

Fixes #2582 #2771 #2610 #754 #2635

This also removes the need to duplicate our presets (react, env etc) in user's app babel config

@codecov
Copy link

codecov bot commented Aug 25, 2018

Codecov Report

Merging #4077 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4077   +/-   ##
=======================================
  Coverage   40.52%   40.52%           
=======================================
  Files         467      469    +2     
  Lines        5636     5636           
  Branches      747      745    -2     
=======================================
  Hits         2284     2284           
- Misses       2982     2984    +2     
+ Partials      370      368    -2
Impacted Files Coverage Δ
lib/cli/lib/helpers.js 0% <ø> (ø) ⬆️
lib/cli/test/fixtures/react_babelrc_js/.babelrc.js 0% <0%> (ø)
app/polymer/src/server/wrapInitialConfig.js 0% <0%> (ø) ⬆️
lib/core/src/server/build-dev.js 0% <0%> (ø) ⬆️
lib/core/src/server/loadCustomBabelConfig.js 0% <0%> (ø) ⬆️
...ib/cli/test/snapshots/react_babelrc_js/.babelrc.js 0% <0%> (ø)
app/angular/src/server/wrapInitialConfig.js 0% <0%> (ø) ⬆️

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 b71117d...9c94d25. Read the comment docs.

@@ -17,27 +15,3 @@ storiesOf('Addons|Knobs/Hello', module)
age,
});
});

storiesOf('Addons|Knobs/Hello (Marko Widget)', module)
Copy link
Member

Choose a reason for hiding this comment

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

There are no widgets in a new version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like they decided to drop this functionality

Copy link
Member

Choose a reason for hiding this comment

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

}

return babelConfig || defaultConfig;
return isBabelLoader8() ? defaultConfig : {};
Copy link
Member

Choose a reason for hiding this comment

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

So, our default babel config is for babel8. After removing babelrc: false, can't we just return the default?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just understood my mistake 🤦‍♂️

@igor-dv
Copy link
Member

igor-dv commented Aug 26, 2018

BTW, by

This also removes the need to duplicate our presets (react, env etc) in user's app babel config

You mean we can get rid of wrapDefaultBabelConfig ?

@@ -19,8 +19,7 @@
},
"dependencies": {
"marko": "^4.12.4",
"marko-starter": "^2.0.4",
"marko-widgets": "^7.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

why delete the widgets?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are deprecated and don't support marko 4

Copy link
Member

Choose a reason for hiding this comment

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

okay, clear!

@Hypnosphi
Copy link
Member Author

You mean we can get rid of wrapDefaultBabelConfig ?

No, it means that our default config is now being merged with user's

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

This is great, this makes so much sense!

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.

3 participants