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

React - use babel presets/plugins based on CRA. #4836

Merged
merged 18 commits into from
Nov 24, 2018

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Nov 23, 2018

Issues:
#4298 = storybook-eol/addon-smart-knobs#33
#4741

What I did

Since CRA's babel configuration is wider than ours, we can use it for the react setup by default, no matter whether the react app is using CRA or not

#4836 (comment)

#4836 (comment)

@igor-dv igor-dv added feature request ci: do not merge react cra Prioritize create-react-app compatibility labels Nov 23, 2018
@igor-dv igor-dv self-assigned this Nov 23, 2018
@codecov
Copy link

codecov bot commented Nov 23, 2018

Codecov Report

Merging #4836 into next will decrease coverage by 0.02%.
The diff coverage is 8.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #4836      +/-   ##
==========================================
- Coverage   35.45%   35.42%   -0.03%     
==========================================
  Files         560      560              
  Lines        6846     6857      +11     
  Branches      919      922       +3     
==========================================
+ Hits         2427     2429       +2     
- Misses       3935     3942       +7     
- Partials      484      486       +2
Impacted Files Coverage Δ
app/react/src/server/options.js 0% <ø> (ø) ⬆️
app/react/src/server/framework-preset-cra.js 0% <0%> (ø)
app/react/src/server/cra-config.js 27.65% <15.38%> (-0.55%) ⬇️

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 b287a99...c331ec8. Read the comment docs.

@igor-dv
Copy link
Member Author

igor-dv commented Nov 23, 2018

Tests will be fixed after #4837

@igor-dv
Copy link
Member Author

igor-dv commented Nov 23, 2018

TBH, I don't like this preset =( We can just add support for the SVGR and dynamic imports independently.
Also, do we want to support SVGR for not CRA apps?

@kylemh
Copy link
Member

kylemh commented Nov 23, 2018

I think SVGR by default for any React app should be safe. If they're justing the import within an img's src attribute, I believe it still works.

@igor-dv
Copy link
Member Author

igor-dv commented Nov 24, 2018

I am not sure about babel-preset-create-app to be used in a non CRA setup, so I just used SVGR and dynamic-imports and moved them to the CRA preset. We can also support dynamic imports on a core basis, but it's only a proposal, so I am not sure about it.

Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

Have you considered using this? It might be a more robust solution than configuring Babel yourself.
https://www.npmjs.com/package/babel-preset-react-app

app/react/src/server/cra-config.js Show resolved Hide resolved
app/react/src/server/cra-config.js Outdated Show resolved Hide resolved
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.

Looks great - tho I agree about the fs comment from @mrmckeb and lots of try/catch... ;-)

@shilman
Copy link
Member

shilman commented Nov 24, 2018

@igor-dv what's the implication of this change? is it a breaking change?

@igor-dv
Copy link
Member Author

igor-dv commented Nov 24, 2018

@mrmckeb I've started from using the babel-preset-create-app, but not sure about using it.
@shilman, should not be breaking change.

I am still experimenting with this, once I am sure it's not breaking anything I will remove the do not merge label

@igor-dv
Copy link
Member Author

igor-dv commented Nov 24, 2018

BTW, there is a side effect of the monorepo, where an official react example is also recognized as using react-scripts 🤦‍♂️

@igor-dv
Copy link
Member Author

igor-dv commented Nov 24, 2018

Looks like babel-preset-react-app is ok. The only thing I am confused with is babel-plugin-transform-react-remove-prop-types that should remove prop-types in prod build. I've checked and prop types still present in stories with the info addon... So idk...

@kylemh
Copy link
Member

kylemh commented Nov 24, 2018

@igor-dv thoughts on this being a CRA2 preset specifically?

@igor-dv
Copy link
Member Author

igor-dv commented Nov 24, 2018

It's a CRA2 specifically, there is a check of the concrete version

@igor-dv igor-dv merged commit 5a85889 into next Nov 24, 2018
@igor-dv igor-dv deleted the react/cra-integration-fixes branch November 24, 2018 20:07
@wiesson
Copy link

wiesson commented Nov 25, 2018

How can I test that feature and give feedback? I have CRA2 with dynamic imports (react-loadable) and I needed to create my own .babelrc due to the required "@babel/plugin-syntax-dynamic-import" babel addon (CRA2 handles all that for me).

@igor-dv
Copy link
Member Author

igor-dv commented Nov 25, 2018

@wiesson
Copy link

wiesson commented Nov 25, 2018

Thanks, I needed to delete node_modules and yarn.lock. Now I got the correct dependencies..

@mrmckeb
Copy link
Member

mrmckeb commented Nov 26, 2018

@igor-dv @wiesson - This should have been available from this release: https://github.com/storybooks/storybook/releases/tag/v4.1.0-alpha.7 - as this is the point where using the CRA2 defaults for CRA2 apps was introduced. Or is it missing something? The new alpha handles this for React apps not based on CRA. Unless I'm missing something?

@igor-dv
Copy link
Member Author

igor-dv commented Nov 26, 2018

alpha 7 brought your TS support for CRA apps
alpha 8 brought SVGR and babel-cra-preset for CRA apps

@mrmckeb
Copy link
Member

mrmckeb commented Nov 26, 2018

Right, @igor-dv. Based on my changes this would have brought support CRA's preset only for TypeScript CRA2.1+ apps.

Did you need help on tests for this @igor-dv? And did you manage to get the react-scripts tests to work? I know CRA doesn't always behave well in monorepos.

@igor-dv
Copy link
Member Author

igor-dv commented Nov 26, 2018

I have added stories for svgr and dynamic imports. They are tested in smoke tests / storyshots. In machine it was working with fallback. But in ci it seems working. I don't want to spend time there for now, so for me this is ok..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cra Prioritize create-react-app compatibility feature request react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants