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

Fix jsconfig support with CRA #9324

Merged
merged 2 commits into from
Jan 31, 2020
Merged

Fix jsconfig support with CRA #9324

merged 2 commits into from
Jan 31, 2020

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Jan 4, 2020

Issue: #9077

#6560 reportedly introduced support for jsconfig.json. Unfortunately, it lacked some important parts of logic used in CRA, like resolving the basePath relative to app directory.

Instead of copy-pasting the full logic from CRA, I just re-use the results from craWebpackConfig

@vercel
Copy link

vercel bot commented Jan 4, 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.

🔍 Inspect: https://zeit.co/storybook/monorepo/jj3znhgbh
✅ Preview: https://monorepo-git-fix-cra-jsconfig.storybook.now.sh

@shilman
Copy link
Member

shilman commented Jan 5, 2020

cc @mrmckeb Does this also need to be applied to @storybook/preset-create-react-app?

@shilman shilman requested a review from mrmckeb January 5, 2020 05:05
@mrmckeb
Copy link
Member

mrmckeb commented Jan 6, 2020

Hi @Hypnosphi, as @shilman noted, this has been deprecated and is now a separate package. I'd rather not introduce any changes here, as we will remove this completely in the next major.

Have you tried the new preset - @storybook/preset-create-react-app? Let us know if that's still an issue there. I am using this on a TypeScript project in a monorepo and it is working there.

https://github.com/storybookjs/presets/blob/master/packages/preset-create-react-app/src/helpers/getModulePath.ts

@Hypnosphi
Copy link
Member Author

I'd rather not introduce any changes here

As long as it's included by default, we should at least fix the bugs

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Jan 6, 2020

@mrmckeb What about all those lines? Are they, for some reason, relevant for user app itself but not for storybook?

https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/config/modules.js#L38-L64

@Hypnosphi Hypnosphi added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jan 6, 2020
@mrmckeb
Copy link
Member

mrmckeb commented Jan 9, 2020

@Hypnosphi, out of interest, are you seeing this in the new preset - or just the deprecated in-built preset?

As long as it's included by default, we should at least fix the bugs
There are many known issues with this preset. @shilman and I discussed this a while ago, and we feel that recommending people to move to the new preset is the best path, as it solves a lot of problems and adds some new features/support.

@Hypnosphi
Copy link
Member Author

Hypnosphi commented Jan 9, 2020

seeing this in the new preset

If you mean #9077, it reproduces in the default setup, i.e. with deprecated preset. I didn't try to reproduce it with the new preset, but I think it should work OK, because you have path.join there.

What concerns me about the new preset is that CRA has some additional logic (linked above), that new preset doesn't include. So I think that getting the actual results from craWebpackConfig is a better option there as well

@mrmckeb
Copy link
Member

mrmckeb commented Jan 12, 2020

The new preset does have additional logic, which resolves conflicts between Storybook and CRAs configs.

I'll leave this to @shilman to make the call - this PR looks fine to me, but again I'd much rather recommend people to move to the new preset (now the default) than to use this one.

@vasilii-kovalev
Copy link

@Hypnosphi, hello! What status of this issue? Are there any difficulties with merging this PR?

@Hypnosphi
Copy link
Member Author

@vasilii-kovalev I don't have any information other than what you can read here

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.

I'll merge this and patch it back to 5.3. We'll remove the built-in CRA preset in 6.0, so as far as I'm concerned it doesn't matter whether the code gets a little more complex @mrmckeb.

@shilman shilman merged commit c589b96 into next Jan 31, 2020
@shilman shilman deleted the fix-cra-jsconfig branch January 31, 2020 12:31
@shilman shilman added this to the 5.3.x milestone Jan 31, 2020
@shilman shilman mentioned this pull request Feb 2, 2020
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Feb 2, 2020
shilman added a commit that referenced this pull request Feb 2, 2020
Fix jsconfig support with CRA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compatibility with other tools configuration babel / webpack cra Prioritize create-react-app compatibility patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants