Skip to content

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

Closed
skevy wants to merge 1 commit into
react:masterfrom
expo:@skevy/local-cli-symlink-fix-upstream
Closed

Refactor how symlinks are discovered in local-cli, support scoped modules#15776
skevy wants to merge 1 commit into
react:masterfrom
expo:@skevy/local-cli-symlink-fix-upstream

Conversation

@skevy

@skevyskevy commented Sep 3, 2017

Copy link
Copy Markdown
Contributor

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 cpojerSeptember 3, 2017 21:50
@facebook-github-botfacebook-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
Copy Markdown
ContributorAuthor

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

@@ -0,0 +1,368 @@
/**

Copy link
Copy Markdown
ContributorAuthor

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.

@skevyskevy changed the title Refactor how symlinks are found in local-cli, support scoped modulesRefactor how symlinks are discovered in local-cli, support scoped modulesSep 3, 2017
@pull-bot

pull-bot commented Sep 3, 2017

Copy link
Copy Markdown
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

skevy commented Sep 3, 2017

Copy link
Copy Markdown
ContributorAuthor

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

c65d904

cc @jeanlauliac@rafeca

@rafeca

Copy link
Copy Markdown
Contributor

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

@jeanlauliacjeanlauliac self-assigned this Sep 4, 2017
@jeanlauliac

Copy link
Copy Markdown

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
skevyforce-pushed the @skevy/local-cli-symlink-fix-upstream branch from 9690dad to be2ffa9CompareSeptember 7, 2017 22:15
@skevy

Copy link
Copy Markdown
ContributorAuthor

Hi @jeanlauliac any updates here?

@facebook-github-botfacebook-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
Copy Markdown
Contributor

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

@jeanlauliacjeanlauliac left a comment

Copy link
Copy Markdown

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)`,

Copy link
Copy Markdown

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

jeanlauliac commented Sep 14, 2017

Copy link
Copy Markdown

@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.
Closesreact/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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Import StartedThis 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

@skevy@pull-bot@rafeca@jeanlauliac@facebook-github-bot