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

[WIP] Make yarn workspaces work #3435

Closed
wants to merge 2 commits into from
Closed

[WIP] Make yarn workspaces work #3435

wants to merge 2 commits into from

Conversation

rmhartog
Copy link

@rmhartog rmhartog commented Nov 9, 2017

First attempt at fixing #3405, by resolving the path using resolve-from, to search up the node_modules hierarchy of the project.

Current status: creating a new project with create-react-app packages/app-a inside a yarn workspaces project succeeds (see #3405), but does not yet detect that it should use yarn. All react-scripts files also need to be updated to respect correct paths.

To do: check for each of the following files how they handle paths and substitute an appropriate solution (probably with resolve-from).

  • create-react-app/
    • createReactApp.js
  • react-scripts/
    • init.js
    • build.js
    • eject.js
    • start.js
    • test.js
    • config/
      • paths.js
  • detect use of yarn in a workspace

I'm aware that we'd rather not edit createReactApp.js, but I'm afraid this fix cannot be made without touching it. Would love to hear more thoughts on this.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@dariocravero
Copy link
Contributor

dariocravero commented Dec 2, 2017

In case anyone is stuck with this and until the different issues are fixed, I made a little guide on how to use yarn workspaces with Create React App and Create React Native App (Expo) to share common code across. Hope you find it handy! https://learn.viewsdx.com/how-to-use-yarn-workspaces-with-create-react-app-and-create-react-native-app-expo-to-share-common-ea27bc4bad62 https://medium.com/viewsdx/how-to-use-yarn-workspaces-with-create-react-app-and-create-react-native-app-expo-to-share-common-ea27bc4bad62

@bradfordlemley
Copy link
Contributor

@rmhartog There's a corner-case issue here when using "yarn link react-scripts" at the top-level workspace (in order to use a create-react-app fork for the workspace). In this case, the app's src files don't actually get used, instead the react-scripts/template files get picked up. This is due to this in react-scripts/config/paths.js:

const reactScriptsLinked =
  fs.existsSync(reactScriptsPath) &&  // <-- doesn't exist for workspace app
  fs.lstatSync(reactScriptsPath).isSymbolicLink();

// config before publish: we're in ./packages/react-scripts/config/
if (
  !reactScriptsLinked &&
  __dirname.indexOf(path.join('packages', 'react-scripts', 'config')) !== -1  <-- true for linked react-scripts
) {
 // use template files <-- oops, I want my src files, not template files

I think the following fixes the issue (and simplifies the logic):

// if appDirectory is the create-react-app repo root, use template files
if (fs.existsSync(path.join(appDirectory, 'packages', 'react-scripts', 'config'))) {
// use template files

BTW, here's a cra-yarn-workspace-example which also documents some test cases which can be used to help verify yarn workspaces support. Your change works great, the only issue I found is the one mentioned here.

@bradfordlemley
Copy link
Contributor

This PR fixes #3031.

@stale
Copy link

stale bot commented Nov 2, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale label Nov 2, 2018
@rmhartog rmhartog closed this Nov 3, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants