-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Future - Create a CRA framework #18695
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit d764cdf. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
@@ -0,0 +1,89 @@ | |||
{ | |||
"name": "@storybook/cra", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to call it cra
or create-react-app
like it was done for the preset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong opinion on this, but a slight preference fro "cra"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylegach thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defer to Norbert.
|
||
## Create React App | ||
|
||
TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO!
77a46f6
to
9d4a720
Compare
18a21a7
to
b2be20f
Compare
b2be20f
to
9682343
Compare
# Conflicts: # addons/docs/src/preset.ts
# Conflicts: # lib/preview-web/src/DocsRender.ts # lib/preview-web/src/PreviewWeb.test.ts # lib/preview-web/src/PreviewWeb.tsx
Looks like this is quite close. Seems like some CSS loader misconfiguration, and some color control acting weird |
# Conflicts: # examples/cra-kitchen-sink/package.json # examples/cra-ts-essentials/package.json # examples/cra-ts-kitchen-sink/package.json # examples/html-kitchen-sink/package.json # lib/cli/src/versions.ts # yarn.lock
Hey @ndelangen shall we merge next into this and check what's the next step? |
options: { | ||
...mdxLoaderOptions, | ||
skipCsf: false, | ||
Object.defineProperty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the idea behind this construct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add a hidden property to the rule object, so it's easier to identify when you want to filter them out
const { presets, configDir } = options; | ||
const frameworkOptions = await presets.apply<FrameworkOptions>('frameworkOptions'); | ||
|
||
const CRAPath = await getCRAPath(frameworkOptions.scriptsPackageName, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capitalization?
), | ||
path.dirname(require.resolve(path.join('@storybook/web-components', 'package.json'))), | ||
dirname(require.resolve(join('@storybook/preset-web-components-webpack', 'package.json'))), | ||
dirname(require.resolve(join('@storybook/web-components', 'package.json'))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use join and not just /
?
|
||
```sh | ||
cd my-react-app | ||
npx sb init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx sb init | |
npx storybook init |
], | ||
"scripts": { | ||
"check": "tsc --noEmit", | ||
"prepare": "esrun ../../scripts/prepare/bundle.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the other frameworks, I think this should be:
"prepare": "esrun ../../scripts/prepare/bundle.ts" | |
"prepare": "esrun ../../../scripts/prepare/bundle.ts" |
@@ -51,6 +51,7 @@ const getFrameworkDetails = ( | |||
builder?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen — FYI, I ended up making a separate PR with this (slightly modified) change: #18968
Replace the
preset-create-react-app
withframework/cra
This framework is what we expect users to use if they use CRA for their app.
This framework has portions of
preset-create-react-app
copied into it, and is self-containing, it does not referencepreset-create-react-app
at all. It's also been significantly reduced in complexity, though this means we have to battle-test it. It's likely not compatible with all version of CRA.I updated all the CRA examples to use this new framework.
I updated the CLI a bit, though this might need more work, I did not test this yet.
Documentation needs more work.
@yannbf raised the question of what this framework should be called..
"@storybook/cra"
vs"@storybook/create-react-app"
?