Add node_module symlink support #1530

mlabrum · 2017-07-27T05:25:33Z

Issue:
If you have a symlink'd node_module then react-native adds that as a project root, as storybook is manually setting the project roots, storybook will need to detect the symlink and add it as a root

I've ran into this issue because we have a monorepo with symlink'd dependencies....

What I did

Edited the storybook start command to call the react-native symlink detection script and add those to the project root

How to test

Move a node module to another location
Create a symlink to that node_module
Ensure that React Native has picked that module up

Thanks!

Specifying project roots on the command line seem to override read natives detection of symlink'd node_modules.

tmeasday · 2017-07-28T00:00:35Z

Could this solve some of the problems we've been having too? #1301

mlabrum · 2017-07-28T00:09:13Z

It could, we're pretty much doing the same thing

packages/app
  - node_modules
      - common -> symlink to ../../common via npm5 file:

packages/common

packages/web
  - node_modules
      - common -> symlink to ../../common via npm5 file:

If you're using windows you'll need to do the following:

Upgrade watchman to a version that supports symlinks on windows
Patch react native Watchman doesn't work with react-native anymore facebook/watchman#479 to support the new watchman paths

If you're using linux/mac the symlink should just work once the packager has the new root.

I've tested this PR and it does solve my problem. (at least on RN 0.45.1)

codecov · 2017-07-28T07:55:32Z

Codecov Report

Merging #1530 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1530      +/-   ##
==========================================
- Coverage   21.41%   21.27%   -0.15%     
==========================================
  Files         244      244              
  Lines        5402     5392      -10     
  Branches      663      656       -7     
==========================================
- Hits         1157     1147      -10     
+ Misses       3761     3760       -1     
- Partials      484      485       +1

Impacted Files	Coverage Δ
app/react-native/src/bin/storybook-start.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/api/actions/api.js	`49.42% <0%> (-1.06%)`	⬇️
lib/ui/src/modules/ui/libs/hierarchy.js	`45.45% <0%> (ø)`	⬆️
.../src/manager/containers/CommentsPanel/dataStore.js	`34.78% <0%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	`6.25% <0%> (ø)`	⬆️
addons/info/src/components/Props.js	`37.2% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/containers/layout.js	`12.5% <0%> (ø)`	⬆️
addons/notes/src/react.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/libs/filters.js	`53.33% <0%> (ø)`	⬆️
app/react-native/src/bin/storybook-build.js	`0% <0%> (ø)`	⬆️
... and 12 more

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 b4968af...f1f73a6. Read the comment docs.

ndelangen · 2017-07-28T07:58:16Z

@mlabrum Thank you so much for this!

Another stepping stone to getting storybook development in a amazing state!

If you could have a look at the linting or add a eslint-ignore-next-line or something we can get this merged I think.

mlabrum · 2017-07-28T08:14:33Z

Hey, I'm glad I could give back :) I'll fix up that linting error Cheers,

…

On Fri, 28 Jul. 2017, 5:28 pm Norbert de Langen, ***@***.***> wrote: @mlabrum <https://github.com/mlabrum> Thank you so much for this! Another stepping stone to getting storybook development in a amazing state! If you could have a look at the linting or add a eslint-ignore-next-line or something we can get this merged I think. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1530 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASEqj-fcY7bhLvucIF67AHysfO7oTb4ks5sSZSkgaJpZM4Okzph> .

atticoos · 2017-07-31T14:09:52Z

This is quite clever, thanks for contributing this! I think many of us run into this problem.

I, in particular, use Haul to gain the benefit of webpack. But having this support-built in is a nice frictionless experience.

Can you think of any scenarios where this might cause problems, as the packager would normally skip symlinks? Wondering if it should be a configuration option that users can opt in or out of.

mlabrum · 2017-08-01T02:11:36Z

@ajwhite this solution pretty much mocks what react native does when it loads its config
https://github.com/facebook/react-native/blob/113e046444e090e38f89574620210ba86b711fed/local-cli/util/Config.js#L153
but it seems by storybook providing its own project root and directories this is ignored within react native.... so storybook has to add these directories.

I'm not sure how Haul would handle these extra project roots....but react natives packager will be fine

ndelangen · 2017-08-02T10:38:58Z

@ajwhite Do you think you could test this for Haul and review & merge?

tmeasday · 2017-08-02T10:48:30Z

I just took a look at this and it works great!

With this change we can remove the RN apps from our list of "non-lerna-ed" things. Perhaps in a follow up PR.

ndelangen · 2017-08-02T10:54:58Z

With this change we can remove the RN apps from our list of "non-lerna-ed" things. Perhaps in a follow up PR.

HOLY SMOKES, I'd say that calls for some 🎊 🎈 🎉 💯

@shilman you nightmare is over!

tmeasday · 2017-08-02T10:57:00Z

Don't get the 🍾 out just yet, bootstrapping was creaking a bit when I tried to test it, but I confirmed that the basic problem (#1301) that caused us to do that was solved by this patch.

Gongreg · 2017-08-07T12:13:30Z

I just have a question:
Is it safe to depend on react-native/local-cli/util/findSymlinksPaths? This is a quite deep import.

danielduan

Looks good!

danielduan · 2017-08-08T22:42:20Z

app/react-native/src/bin/storybook-start.js


+  const projectRoots = (configDir === projectDir ? [configDir] : [configDir, projectDir]).concat(symlinks);


I think it'd be a good option somewhere to be able to specify a different project root through storybook-start for those who have components and assets that need to be included from outside of the storybook root project.

It's definitely out of scope of this PR though so don't feel obligated to add this by any means. If it's a feature we want, I think a separate issue is an appropriate place to start.

I agree, it would be nice to have, but probably belongs in a different PR

mlabrum · 2017-08-09T00:57:48Z

@Gongreg It's as safe as it could be, I've wrapped it in a try/catch...the only option around this would be to copy their code into the repo.

it's probably the same level of risk as depending on node_modules/react-native/local-cli/cli.js

ndelangen · 2017-08-10T15:21:51Z

@tmeasday Are you ready to approve this?

tmeasday · 2017-08-12T10:35:58Z

Yes!

ndelangen · 2017-08-12T23:40:20Z

Thank you @mlabrum for this PR!

Add node_module symlink support

a78ab69

Specifying project roots on the command line seem to override read natives detection of symlink'd node_modules.

ndelangen requested review from orta, tmeasday, Gongreg and a team July 27, 2017 22:22

ndelangen added the react-native label Jul 27, 2017

tmeasday self-assigned this Jul 28, 2017

Merge branch 'master' into patch-1

b9a904d

ndelangen added the maintenance User-facing maintenance tasks label Jul 28, 2017

mlabrum added 2 commits July 31, 2017 09:22

Fix lint errors, handle case where findSymlinksPaths doesn't exist

dce4b4b

Merge branch 'master' into patch-1

e5fc81d

Merge branch 'master' into patch-1

3c84840

ndelangen approved these changes Aug 2, 2017

View reviewed changes

danielduan approved these changes Aug 8, 2017

View reviewed changes

Gongreg approved these changes Aug 9, 2017

View reviewed changes

Merge branch 'master' into patch-1

2fe3a6f

tmeasday approved these changes Aug 12, 2017

View reviewed changes

Tom Coleman and others added 3 commits August 12, 2017 20:36

Merge branch 'master' into patch-1

dd13e04

Merge branch 'master' into patch-1

3b7efda

Merge branch 'master' into patch-1

f1f73a6

ndelangen merged commit 8fcdbe7 into storybookjs:master Aug 12, 2017

k0nserv mentioned this pull request Sep 18, 2017

Support specifying the react-native cli config #1861

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node_module symlink support #1530

Add node_module symlink support #1530

mlabrum commented Jul 27, 2017

tmeasday commented Jul 28, 2017

mlabrum commented Jul 28, 2017 •

edited

Loading

codecov bot commented Jul 28, 2017 •

edited

Loading

ndelangen commented Jul 28, 2017

mlabrum commented Jul 28, 2017 via email

atticoos commented Jul 31, 2017 •

edited

Loading

mlabrum commented Aug 1, 2017

ndelangen commented Aug 2, 2017

tmeasday commented Aug 2, 2017

ndelangen commented Aug 2, 2017 •

edited

Loading

tmeasday commented Aug 2, 2017 •

edited

Loading

Gongreg commented Aug 7, 2017

danielduan left a comment

danielduan Aug 8, 2017 •

edited

Loading

mlabrum Aug 9, 2017

mlabrum commented Aug 9, 2017

ndelangen commented Aug 10, 2017

tmeasday commented Aug 12, 2017

ndelangen commented Aug 12, 2017


		const projectRoots = (configDir === projectDir ? [configDir] : [configDir, projectDir]).concat(symlinks);

Add node_module symlink support #1530

Add node_module symlink support #1530

Conversation

mlabrum commented Jul 27, 2017

What I did

How to test

tmeasday commented Jul 28, 2017

mlabrum commented Jul 28, 2017 • edited Loading

codecov bot commented Jul 28, 2017 • edited Loading

Codecov Report

ndelangen commented Jul 28, 2017

mlabrum commented Jul 28, 2017 via email

atticoos commented Jul 31, 2017 • edited Loading

mlabrum commented Aug 1, 2017

ndelangen commented Aug 2, 2017

tmeasday commented Aug 2, 2017

ndelangen commented Aug 2, 2017 • edited Loading

tmeasday commented Aug 2, 2017 • edited Loading

Gongreg commented Aug 7, 2017

danielduan left a comment

Choose a reason for hiding this comment

danielduan Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

mlabrum Aug 9, 2017

Choose a reason for hiding this comment

mlabrum commented Aug 9, 2017

ndelangen commented Aug 10, 2017

tmeasday commented Aug 12, 2017

ndelangen commented Aug 12, 2017

mlabrum commented Jul 28, 2017 •

edited

Loading

codecov bot commented Jul 28, 2017 •

edited

Loading

atticoos commented Jul 31, 2017 •

edited

Loading

ndelangen commented Aug 2, 2017 •

edited

Loading

tmeasday commented Aug 2, 2017 •

edited

Loading

danielduan Aug 8, 2017 •

edited

Loading