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

[WIP] presets - merge default babel configs #4107

Merged

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Sep 2, 2018

Issue: This PR is based on the #4043 (for the reviewability)

What I did

I've removed wrapDefaultBabelConfig in apps, and added the ability to merge babel configs. This will ease the app creation process and presets usage

…o core/presets-merge-babel-configs

# Conflicts:
#	lib/core/package.json
#	lib/core/src/server/core-preset-dev.js
#	lib/core/src/server/core-preset-prod.js
#	yarn.lock
@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is marked with "do not merge", "in progress" labels.

🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Sep 2, 2018

Codecov Report

Merging #4107 into core/framework-core-presets will decrease coverage by 0.25%.
The diff coverage is 0%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           core/framework-core-presets    #4107      +/-   ##
===============================================================
- Coverage                        40.74%   40.49%   -0.26%     
===============================================================
  Files                              491      491              
  Lines                             5794     5830      +36     
  Branches                           779      793      +14     
===============================================================
  Hits                              2361     2361              
- Misses                            3058     3080      +22     
- Partials                           375      389      +14
Impacted Files Coverage Δ
app/vue/src/server/options.js 0% <ø> (ø) ⬆️
.../react/src/server/framework-preset-react-docgen.js 100% <ø> (ø)
app/mithril/src/server/options.js 0% <ø> (ø) ⬆️
app/react/src/server/options.js 0% <ø> (ø) ⬆️
lib/core/src/server/babel-core-proxy.js 0% <0%> (ø)
lib/core/src/server/core-preset-dev.js 0% <0%> (ø) ⬆️
lib/core/src/server/mergeBabel.js 0% <0%> (ø)
lib/core/server.js 0% <0%> (ø) ⬆️
app/vue/src/server/framework-preset-vue.js 0% <0%> (ø) ⬆️
lib/core/src/server/config.js 0% <0%> (ø) ⬆️
... and 6 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 05e6438...209fe99. Read the comment docs.

@igor-dv igor-dv merged commit ed69634 into core/framework-core-presets Sep 3, 2018
@igor-dv igor-dv deleted the core/presets-merge-babel-configs branch September 3, 2018 15:01
const { configDir, wrapDefaultBabelConfig = noopWrapper } = options;

const babelOptions = presets.extendBabel({}, { configDir, wrapDefaultBabelConfig });
const babelOptions = presets.extendBabel({}, options);
Copy link
Member

@Hypnosphi Hypnosphi Sep 10, 2018

Choose a reason for hiding this comment

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

This breaks Babel 6 support, because it adds our defaults (which are Babel 7 only) to all configs

https://teamcity.jetbrains.com/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=1602700&_focus=1075

Error: Requires Babel "^7.0.0-0", but was loaded with "6.26.3". If you are sure you have a compatible version of @babel/core, it is likely that something in your build process is loading the wrong version. Inspect the stack trace of this error to look for the first entry that doesn't mention "@babel/core" or "babel-core" to see what is calling Babel. (While processing preset: "/mnt/agent/work/a1796458d49a7849/node_modules/@babel/preset-react/lib/index.js")
[00:16:35][Step 2/2]     at throwVersionError (/mnt/agent/work/a1796458d49a7849/node_modules/@babel/helper-plugin-utils/lib/index.js:65:11)
[00:16:35][Step 2/2]     at Object.assertVersion (/mnt/agent/work/a1796458d49a7849/node_modules/@babel/helper-plugin-utils/lib/index.js:13:11)
[00:16:35][Step 2/2]     at _default (/mnt/agent/work/a1796458d49a7849/node_modules/@babel/preset-react/lib/index.js:61:7)

Copy link
Member

@Hypnosphi Hypnosphi Sep 10, 2018

Choose a reason for hiding this comment

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

We really need separate extendDefaultBabel and extendBabel. The former should only be applied to our defaults (which aren't applied for Babel 6), and the latter to final config

Copy link
Member

Choose a reason for hiding this comment

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

Actually, react-docgen is probably the only usecase for the latter

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean react-docgen is the only working usecase?

Copy link
Member

@Hypnosphi Hypnosphi Sep 10, 2018

Choose a reason for hiding this comment

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

It's the only plugin that we need to add to any configuration (using wrapBabelConfig) because it typically isn't used in user app. All the other stuff should only be added to default babel config.

Maybe we could add react-docgen in app/react's extendWebpack to avoid two different babel extenders

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