Skip to content

Conversation

@mrmckeb
Copy link
Member

@mrmckeb mrmckeb commented Nov 4, 2018

Issue: #4681

What I did

Added support for react-scripts packages with names other than react-scripts.

How to test

Create a fork of react-scripts with a different name (or use an existing), and use that for your --scripts-version when running create-react-app.

@storybook-safe-bot
Copy link
Contributor

storybook-safe-bot commented Nov 4, 2018

Fails
🚫

PR is marked with "feature request" label.

Generated by 🚫 dangerJS

@Hypnosphi Hypnosphi changed the base branch from master to next November 5, 2018 17:04
Copy link
Member

@chadfawcett chadfawcett left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

We need some CLI test fixture for this IMO

@igor-dv igor-dv added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Nov 8, 2018
@ndelangen ndelangen self-assigned this Nov 9, 2018
@ndelangen ndelangen added this to the next milestone Nov 9, 2018
@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #4712 into next will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #4712      +/-   ##
==========================================
+ Coverage   35.51%   35.55%   +0.04%     
==========================================
  Files         557      557              
  Lines        6775     6739      -36     
  Branches      901      885      -16     
==========================================
- Hits         2406     2396      -10     
+ Misses       3899     3882      -17     
+ Partials      470      461       -9
Impacted Files Coverage Δ
app/react/src/server/cra_config.js 0% <0%> (ø) ⬆️
lib/components/src/layout/desktop.js 73.68% <0%> (-0.46%) ⬇️
...ents/stories_panel/stories_tree/tree_decorators.js 100% <0%> (ø) ⬆️
...mponents/stories_panel/stories_tree/tree_header.js 100% <0%> (ø) ⬆️
...ddons/storyshots/storyshots-puppeteer/src/index.js 0% <0%> (ø) ⬆️
addons/jest/src/index.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
lib/components/src/header/header.js 100% <0%> (ø) ⬆️
lib/core/src/server/build-static.js 0% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33d6091...290eacd. Read the comment docs.

@ndelangen
Copy link
Member

I agree with @igor-dv
I think we can merge this though.

Would you be able to open another PR @mrmckeb that adds some tests?

@ndelangen ndelangen merged commit 911098b into storybookjs:next Nov 9, 2018
@mrmckeb mrmckeb deleted the feature/custom-react-scripts branch November 9, 2018 09:14
@mrmckeb
Copy link
Member Author

mrmckeb commented Nov 9, 2018

Certainly @igor-dv and @ndelangen, I'll take a look at doing this over the weekend. Hopefully it'll be fairly straight-forward to test.

And thanks for your review too @chadfawcett.

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Nov 13, 2018
shilman pushed a commit that referenced this pull request Nov 13, 2018
Add support for custom `react-scripts` packages
@igor-dv
Copy link
Member

igor-dv commented Nov 24, 2018

I've played a bit with this in #4836 and It doesn't find for me the react-scripts pkg in the cra-kitchen-sink example. Might It be related either to monorepo or windows?

@igor-dv igor-dv added the cra Prioritize create-react-app compatibility label Nov 24, 2018
@mrmckeb
Copy link
Member Author

mrmckeb commented Nov 24, 2018

@igor-dv, this is working for my team on Windows (WSL) and MacOS. Can you give more details of what you're seeing?

@igor-dv
Copy link
Member

igor-dv commented Nov 24, 2018

When I am debuggin this:

let reactScriptsPath;

function getReactScriptsPath() {
  if (reactScriptsPath) return reactScriptsPath;
  const appDirectory = fs.realpathSync(process.cwd());
  const reactScriptsScriptPath = fs.realpathSync(
    path.join(appDirectory, '/node_modules/.bin/react-scripts')
  );
  reactScriptsPath = path.join(reactScriptsScriptPath, '../..');
  return reactScriptsPath;
}

in cra-kitchen-sink, I see that

reactScriptsScriptPath = 'root\examples\cra-kitchen-sink\node_modules\.bin\react-scripts'

Then

reactScriptsPath = 'root\examples\cra-kitchen-sink\node_modules'

And after when we are tring to use `isReactScriptsInstalled that does

require(path.join(getReactScriptsPath(), 'package.json'));

path to package.json is wrong and doesn't point to the package.json in the react-scripts

@mrmckeb
Copy link
Member Author

mrmckeb commented Nov 24, 2018

And does it work for you outside of that environment @igor-dv? And are you using WSL? I'm not home now, but can take a look later.

@igor-dv
Copy link
Member

igor-dv commented Nov 24, 2018

Didn't check it outside. I am just using a regular windows 10

@igor-dv
Copy link
Member

igor-dv commented Nov 24, 2018

Even tests (cra-config) are failing locally for me, but passing in CI. Probably a windows thing =(

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

Labels

compatibility with other tools cra Prioritize create-react-app compatibility feature request 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.

7 participants