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

fixed using wrong package manager in yarn workspace projects #503

Merged
merged 21 commits into from Mar 6, 2018

Conversation

connectdotz
Copy link
Contributor

this is to fix react-native-scripts not able to find yarn.lock file in the monorepo (workspaces) projects, which caused the react-native-scripts to use the wrong package manager and failed.

yarn workspace project puts the yarn.lock in the project root instead of the react native app directory. Since the current implementation only looks for yarn.lock in the app/cwd directory, it doesn't see the file and will wrongly select npm as the package manager. The new lookup method will traverse up the filesystem (similar to node module resolution) until it either finds the file or hits the filesystem root.

outline the changes:

  1. consolidated all yarn.lock lookup into a new file react-native-scripts/src/utils/pm.js, which performs the new logic described above.
  2. setup jest testing. Future contributors can run yarn test to guard against regression. Also updated taskfile.js to exclude tests in the build.
  3. added test for the new logic

also setup jest test so we can start building more tests
@connectdotz connectdotz changed the title fixed yarn.lock file lookup for yarn workspace projects fixed using wrong package manager in yarn workspace projects Dec 8, 2017
@anp anp requested a review from fson January 3, 2018 01:15
Copy link
Contributor

@fson fson left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting this reviewed. This change seems reasonable to me.

Thanks for also adding a test case. I had some minor comments about Jest configuration, but otherwise this seems ready to be merged.

package.json Outdated
},
"jest": {
"moduleFileExtensions": [ "js" ],
"testRegex": "(/__tests__/.*|\\.(test|spec))\\.(js)$",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these are the default settings in Jest now, so we should be able to omit moduleFileExtensions and testRegex.

package.json Outdated
"moduleFileExtensions": [ "js" ],
"testRegex": "(/__tests__/.*|\\.(test|spec))\\.(js)$",
"testPathIgnorePatterns": [
"\\.snap$",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be omitted.

@connectdotz
Copy link
Contributor Author

done

@connectdotz
Copy link
Contributor Author

ping

@fson
Copy link
Contributor

fson commented Mar 6, 2018

Hey, looks like something went wrong with merging changes from master. Could you please rebase this branch onto master?

@connectdotz
Copy link
Contributor Author

rebased

@fson fson merged commit ed1603c into expo:master Mar 6, 2018
fson added a commit that referenced this pull request Mar 6, 2018
fixed using wrong package manager in yarn workspace projects
@fson
Copy link
Contributor

fson commented Mar 6, 2018

Thanks @connectdotz!

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

Successfully merging this pull request may close these issues.

None yet

8 participants