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

Forked CRA support on Windows #6236

Merged
merged 2 commits into from
Mar 23, 2019
Merged

Forked CRA support on Windows #6236

merged 2 commits into from
Mar 23, 2019

Conversation

sergeyzwezdin
Copy link
Contributor

Issue: Support for CRA fork packages on Windows (#5801)

What I did

I've added parsing of .bin/react-scripts file to find out actual react-scripts package path.

@shilman shilman added react cra Prioritize create-react-app compatibility bug labels Mar 22, 2019
@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #6236 into next will decrease coverage by <.01%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6236      +/-   ##
==========================================
- Coverage   38.12%   38.12%   -0.01%     
==========================================
  Files         645      645              
  Lines        9646     9654       +8     
  Branches      355      355              
==========================================
+ Hits         3678     3681       +3     
- Misses       5913     5918       +5     
  Partials       55       55
Impacted Files Coverage Δ
app/react/src/server/cra-config.js 81.25% <44.44%> (-6.25%) ⬇️

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 4057939...2540eba. Read the comment docs.

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.

Any chance you can add a unit test for this in cra-config.test.js? This is hard to test without a windows machine and inside our monorepo which doesn't have any CRA-fork apps. But I'd feel a lot better about merging if I knew what the data looked like.

@sergeyzwezdin
Copy link
Contributor Author

@shilman Hm, I would happy to add that, but I do not understand how to emulate .bin and node_modules folder in the test. If you advise me, I will add it for sure.

@shilman
Copy link
Member

shilman commented Mar 22, 2019

@sergeyzwezdin if you look at the existing tests, the fs module is already mocked, so you can make it return whatever you want in your test. Make it return something that looks like what you'd get on the windows machine, and verify that the result of your new code is what you'd expect. Does that make sense? I think you can just follow the patterns that are already in the test file.

@sergeyzwezdin
Copy link
Contributor Author

@shilman Sounds good. I'll try it tomorrow morning.

@shilman
Copy link
Member

shilman commented Mar 22, 2019

Great @sergeyzwezdin. Thanks for your patience on this and I'll get it merged and released on alpha as soon as I'm comfortable with the change.

@shilman shilman added this to the 5.0.x milestone Mar 22, 2019
@sergeyzwezdin
Copy link
Contributor Author

@shilman Ok, I managed to add the test. It seems everything is 👌now.

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.

Looks super, great job @sergeyzwezdin! 🚀 I'll release this on alpha today (and post on the issue) and then will patch it back into master once you test it out on the alpha. Sound ok?

@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Mar 23, 2019
@shilman shilman merged commit 4d82e4d into storybookjs:next Mar 23, 2019
@sergeyzwezdin
Copy link
Contributor Author

@shilman yep, sounds cool! 👍🏼

@shilman
Copy link
Member

shilman commented Mar 23, 2019

@sergeyzwezdin ready for testing 🔍

@sergeyzwezdin
Copy link
Contributor Author

@shilman Works like a charm. When do you plan to release it?

image

@shilman
Copy link
Member

shilman commented Mar 23, 2019

Super. I've been doing alpha releases every other day (ish) and stable releases once or twice a week (ish) so probably tomorrow. I'll post on the issue when it's live

@sergeyzwezdin
Copy link
Contributor Author

sergeyzwezdin commented Mar 23, 2019

Cool! Waiting for that then. Thank you for your help.

@shilman shilman added patch:done Patch/release PRs already cherry-picked to main/release branch and removed patch:done Patch/release PRs already cherry-picked to main/release branch labels Mar 23, 2019
shilman added a commit that referenced this pull request Mar 23, 2019
Forked CRA support on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 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.

2 participants