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

Core: Support custom addons using JSX #9648

Merged
merged 19 commits into from Feb 7, 2020
Merged

Core: Support custom addons using JSX #9648

merged 19 commits into from Feb 7, 2020

Conversation

ndelangen
Copy link
Member

Issue: #9421

What I did

MOVE the babel-loader_function to preview folder
ADD a COPY of tailored babel-loader_function for the manager

The tailored babel-loader_function adds presets & plugins for react, it makes it so the babel config is like what we use on our /addons.

Why

See the bug.
A user is using storybook for svelte, and so they do not have @babel-preset-react in their babel config.
But now in their custom addon, there's JSX, which is not transpiled and so webpack throws and error.

How to test

To test this effectively, one should add a custom addon from 'manager.js' using something like JSX in it.

Without this change, storybook start will fail, because it will find non-js in source it cannot put into the bundle.

With this change storybook start will succeed, because the correct babel-presets & babel-plugins are added

…or manager

The big change is that now, the babel presets & plugins used for storybook (react) code are ensured for manager injected code

This means addons referenced from preview.js & manager.js will load with babel config including react preset, etc.
@ndelangen ndelangen added bug configuration babel / webpack patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Jan 27, 2020
@ndelangen ndelangen added this to the 5.3.x milestone Jan 27, 2020
@ndelangen ndelangen self-assigned this Jan 27, 2020
@vercel
Copy link

vercel bot commented Jan 27, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

I think emotion & decorators are a bit too opinionated to be injected into everyone's babel config.
@ndelangen
Copy link
Member Author

I found that merging the babel config is dangerous, so I'm throwing out the config the user provided and using just the config I know will work for the manager runtime.

This unfortunately doesn't work when the user is using babel 6.

#9675

@@ -27,7 +27,6 @@ export default ({
dll,
outputDir,
cache,
babelOptions,
Copy link
Member

Choose a reason for hiding this comment

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

why remove?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, i saw your comment above

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.

LGTM tho I don't 100% get it. also CLI tests are failing.

@ndelangen
Copy link
Member Author

Yes the CLI test are failing on: Running smoke test in react_babel_6

I'm considering dropping support for babel 6 in storybook 6.0.0.

Then the CLI tests would pass.

@shilman
Copy link
Member

shilman commented Feb 6, 2020

I'm fine with removing babel 6 support in 6.0

cc @tmeasday @Hypnosphi @igor-dv

# Conflicts:
#	lib/core/package.json
#	lib/core/src/server/manager/manager-webpack.config.js
#	lib/core/src/server/preview/iframe-webpack.config.js
@ndelangen ndelangen removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Feb 6, 2020
@ndelangen ndelangen modified the milestones: 5.3.x, 6.0.0 Feb 6, 2020
@ndelangen
Copy link
Member Author

Argh, some problem in the marko example :(

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@ndelangen looks like a bunch of the CLI tests are using addon-notes, which is failing. any idea what's going on there?

@ndelangen
Copy link
Member Author

@shilman must have been some merge conflict. I fixed that on next & merged next into this branch. I think it's all good now.

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@ndelangen nice work. thanks for getting this over the line!

@shilman shilman changed the title FIX 9421 - custom addons using JSX Core: Support custom addons using JSX Feb 7, 2020
@shilman shilman merged commit beb3137 into next Feb 7, 2020
@shilman shilman deleted the fix/9421-local-addons branch February 7, 2020 14:16
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.

None yet

3 participants