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

getReactScriptsPath not detecting my forked react-scripts package #5801

Closed
d3bgger opened this issue Feb 28, 2019 · 10 comments
Closed

getReactScriptsPath not detecting my forked react-scripts package #5801

d3bgger opened this issue Feb 28, 2019 · 10 comments

Comments

@d3bgger
Copy link

d3bgger commented Feb 28, 2019

I'm trying to make storybook detect my forked cra package (based on cra 2.1.5) with no luck.

The following code in getReactScriptsPath() (https://github.com/storybooks/storybook/blob/be544bc22043da83300523b59c7734ba54db1eae/app/react/src/server/cra-config.js#L13) always returns the hardcoded 'react-scripts' because it always trying to find a package.json in node_modules folder (2 folders up from /.bin/react-scripts). How is this supposed to work?

`const reactScriptsScriptPath = _fs.default.realpathSync(_path.default.join(appDirectory, '/node_modules/.bin/react-scripts'));

reactScriptsPath = _path.default.join(reactScriptsScriptPath, '../..');

const scriptsPkgJson = _path.default.join(reactScriptsPath, 'package.json');

if (!_fs.default.existsSync(scriptsPkgJson)) {
reactScriptsPath = 'react-scripts';
}`

thanks in advance

@stale
Copy link

stale bot commented Mar 21, 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 Mar 21, 2019
@sergeyzwezdin
Copy link
Contributor

sergeyzwezdin commented Mar 22, 2019

Hello!

We also use forked CRA with storybook.

As @d3bgger mentioned the problem with realpathSync:

  1. On Mac it works like a charm because of node_modules/.bin/react-scripts is a symlink. So realpathSync resolves the path to target package correctly.
  2. On Windows though it has some problems. Since node_modules/.bin/react-scripts is not symlink it doesn't resolve the target path correctly. As a result, Storybook tries to find it as node_modules/react-scripts which is wrong (there is no react-scripts folder since we have our custom package name). So, Storybook displays warning that it uses default webpack config, not from CRA.
info => Using base config because react-scripts is not installed.

image

So, need to fix it somehow to find a real path of react-scripts on Windows.

@shilman you've marked the issue as "feature request", but it's actually the bug.

@stale stale bot removed the inactive label Mar 22, 2019
@shilman
Copy link
Member

shilman commented Mar 22, 2019

@sergeyzwezdin Does it work with CRA on Windows? If not, then it's a bug. If it works with CRA and does not work with a CRA fork, then it's a feature request since we don't claim to support that use case. As for relpathSync, if you have any workarounds we'd be more than happy to take a PR to support this.

@d3bgger
Copy link
Author

d3bgger commented Mar 22, 2019

@sergeyzwezdin Does it work with CRA on Windows? If not, then it's a bug. If it works with CRA and does not work with a CRA fork, then it's a feature request since we don't claim to support that use case. As for relpathSync, if you have any workarounds we'd be more than happy to take a PR to support this.

Isn't this the PR adding support for CRA forks?
#4712

@sergeyzwezdin
Copy link
Contributor

Isn't this the PR adding support for CRA forks?

Yes, but it assumes that .bin/react-scripts is symlink which is wrong for Windows.

I've created another PR #6235, hope you'll merge it.

@shilman
Copy link
Member

shilman commented Mar 22, 2019

@sergeyzwezdin Awesome, thanks so much for this! Reviewing it now 👍

@shilman
Copy link
Member

shilman commented Mar 22, 2019

@d3bgger @sergeyzwezdin Didn't realize that we already were trying to support this. Bug it is then 😄

@shilman
Copy link
Member

shilman commented Mar 23, 2019

Yowza!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.13 containing PR #6236 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

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

@shilman shilman closed this as completed Mar 23, 2019
@shilman
Copy link
Member

shilman commented Mar 24, 2019

Yee-haw!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.4 containing PR #6236 that references this issue. Upgrade today to try it out!

@sergeyzwezdin
Copy link
Contributor

@shilman Thank you for quick release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants