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

Refactor how symlinks are discovered in local-cli, support scoped modules #15776

Closed

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Sep 3, 2017

This PR refactors the symlink finding logic in local-cli in order to support nested symlinked modules as well as symlinked scoped NPM modules.

Test Plan

Run tests, or try project with npm link'ed or yarn link'ed dependencies.

@skevy skevy requested a review from cpojer September 3, 2017 21:50
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Sep 3, 2017
path.join(getProjectPath(), 'node_modules'),
roots
)
const resolveSymlinksForRoots = roots =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds support for resolving symlinks in multiple roots, rather than just the project path

@cpojer cpojer requested a review from jeanlauliac September 3, 2017 21:52
@@ -0,0 +1,368 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of the fs mock from metro-bundler. Adds exists/existsSync + readlink/readlinkSync support.

@skevy skevy changed the title Refactor how symlinks are found in local-cli, support scoped modules Refactor how symlinks are discovered in local-cli, support scoped modules Sep 3, 2017
@pull-bot
Copy link

pull-bot commented Sep 3, 2017

Warnings
⚠️

❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 850 lines.

@facebook-github-bot label Core Team

@facebook-github-bot large-pr

Generated by 🚫 dangerJS

@skevy
Copy link
Contributor Author

skevy commented Sep 3, 2017

Flow error from Circle is unrelated. Was introduced 2 days ago here:

c65d904

cc @jeanlauliac @rafeca

@rafeca
Copy link
Contributor

rafeca commented Sep 3, 2017

@skevy tomorrow we are going to release a new version of metro-bundler to fix the flow issue

@jeanlauliac jeanlauliac self-assigned this Sep 4, 2017
@jeanlauliac
Copy link

I'll try to make progress on that today/tomorrow

This PR refactors the symlink finding logic in order to support nested symlinked modules as well as scoped NPM modules.
@skevy skevy force-pushed the @skevy/local-cli-symlink-fix-upstream branch from 9690dad to be2ffa9 Compare September 7, 2017 22:15
@skevy
Copy link
Contributor Author

skevy commented Sep 12, 2017

Hi @jeanlauliac any updates here?

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 13, 2017
@facebook-github-bot
Copy link
Contributor

@jeanlauliac has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link

@jeanlauliac jeanlauliac left a comment

Choose a reason for hiding this comment

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

Yup looks good to me, thank you! I'll try to merge today. Sorry for the delay, I'll try to be more reactive on PRs here.


console.log(
`Scanning folders for symlinks in ${nodeModuleRoot} (${timeEnd -
timeStart}ms)`,

Choose a reason for hiding this comment

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

Normally we'd want to go through a reporter for any output, not print to the console directly. But I'm okay with shipping it like so and we can improve later :)

@jeanlauliac
Copy link

jeanlauliac commented Sep 14, 2017

@skevy lemme know if that looks all good 😄

ide pushed a commit that referenced this pull request Sep 14, 2017
…ules

Summary:
This PR refactors the symlink finding logic in local-cli in order to support nested symlinked modules as well as symlinked scoped NPM modules.

Run tests, or try project with `npm link`'ed or `yarn link`'ed dependencies.
Closes #15776

Reviewed By: cpojer

Differential Revision: D5823008

Pulled By: jeanlauliac

fbshipit-source-id: f2daeceef37bed2f8a136a0a5918730f9832884c
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
…ules

Summary:
This PR refactors the symlink finding logic in local-cli in order to support nested symlinked modules as well as symlinked scoped NPM modules.

Run tests, or try project with `npm link`'ed or `yarn link`'ed dependencies.
Closes facebook/react-native#15776

Reviewed By: cpojer

Differential Revision: D5823008

Pulled By: jeanlauliac

fbshipit-source-id: f2daeceef37bed2f8a136a0a5918730f9832884c
@ide ide deleted the @skevy/local-cli-symlink-fix-upstream branch April 25, 2020 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants