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

CRA: Move cra-config into a separate preset #7221

Closed
3 tasks done
shilman opened this issue Jun 28, 2019 · 13 comments · Fixed by storybookjs/presets#25
Closed
3 tasks done

CRA: Move cra-config into a separate preset #7221

shilman opened this issue Jun 28, 2019 · 13 comments · Fixed by storybookjs/presets#25
Assignees
Labels
cra Prioritize create-react-app compatibility feature request presets

Comments

@shilman
Copy link
Member

shilman commented Jun 28, 2019

Currently CRA config is a magic step that happens during @storybook/react loading. It should be split out into its own preset so that it is more transparent and easier to reason about.

For example in storybookjs/presets#20, CRA is silently removing the docgen information added by preset-typescript.

There are also other cases, e.g. supporting custom forks of CRA, where we want to give users more control: #6973

It would be great to:

  • Move CRA-config into its own preset
  • Auto-detect react-scripts in the CLI and add that preset automatically
  • Provide a clean migration path for existing CRA apps (a warning if preset is not defined + instructions in MIGRATION.md?)

In addition (probably a separate issue?) preset-cra should log when it's removing rules using #7220.

@mrmckeb
Copy link
Member

mrmckeb commented Jul 1, 2019

Hey @shilman, I could possibly pick this up over the weekend?

We'll have to think about how to handle customisation - I think we could probably just say that if you've modified CRA, you'll probably need to modify this config (or write your own).

@stale
Copy link

stale bot commented Jul 22, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jul 22, 2019
@mrmckeb mrmckeb removed the inactive label Jul 22, 2019
@shilman
Copy link
Member Author

shilman commented Jul 22, 2019

Hey @mrmckeb I think this would be a big milestone towards getting presets adopted. We're in the thick of the 5.2 release, so we probably wouldn't get it in until 5.3, but if you wanted to take a stab at it, it would be much appreciated!! 🙇

@mrmckeb
Copy link
Member

mrmckeb commented Jul 25, 2019

Thanks @shilman, I think this weekend I can start looking at it ;)

@mrmckeb
Copy link
Member

mrmckeb commented Jul 28, 2019

I've started on this today @shilman, I'll try to get it done in the next 7-10 days :)

@shilman
Copy link
Member Author

shilman commented Jul 28, 2019

Awesome!!! Thanks for taking this on. I'm really excited about where this is headed and have been doing some interesting preset work in docs-land. I think we can make this really sweet 🔥

@mrmckeb mrmckeb self-assigned this Jul 28, 2019
@mrmckeb
Copy link
Member

mrmckeb commented Jul 28, 2019

@shilman In case you see this first, I pinged you for a quick discussion on this ;)

@shilman
Copy link
Member Author

shilman commented Oct 9, 2019

Migration path for CRA users:

5.2 (master): if user adds CRA preset, we use that. if not, we use the built-in CRA hack. sb init does nothing special in a CRA project.

5.3 (next): if user adds CRA preset, we use that. if not, we use the CRA preset automatically and maybe log a warning that it will be required in 6.0. sb init adds the CRA preset automatically in presets.js.

6.0: if user adds CRA preset, we use that. if not, log a warning that CRA preset is required in 6.0. sb init adds the CRA preset automatically in presets.js.

@mrmckeb
Copy link
Member

mrmckeb commented Oct 9, 2019

Thanks @shilman, so in short, we 5.3 progresses as is - but use the preset if it exists. In 6.0, we require the preset.

@shilman
Copy link
Member Author

shilman commented Oct 10, 2019

@mrmckeb The diff between 5.2 and 5.3 in my proposal is that 5.2 uses the built-in preset, whereas 5.3 uses your preset package. If that's possible?

@mrmckeb
Copy link
Member

mrmckeb commented Oct 14, 2019

Ah, OK - yes... will take a look today.

@shilman
Copy link
Member Author

shilman commented Oct 14, 2019

Jiminy cricket!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.19 containing PR #8416 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman
Copy link
Member Author

shilman commented Oct 17, 2019

Huzzah!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.21 containing PR #8449 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

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 presets
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants