[RNPM] Add pre/postunlink #9157

geof90 · 2016-08-02T07:42:28Z

An attempt to address #9156.

Test plan (required)

Added the following to the package.json of a react-native-plugin

  "rnpm": {
    "commands": {
      "preunlink": "node node_modules/react-native-plugin/scripts/preunlink"
      "postunlink": "node node_modules/react-native-plugin/scripts/postunlink"
    }
  }

Added files, scripts/preunlink.js and scripts/postunlink.js to the plugin. Each of them simply logs a string to the console.
Ran react-native unlink react-native-plugin to verify that those logs get printed.

ghost · 2016-08-02T07:44:40Z

@geof90 updated the pull request.

geof90 · 2016-08-02T07:44:47Z

local-cli/cliEntry.js

@@ -137,7 +137,7 @@ function run() {

  commander.parse(process.argv);

-  const isValidCommand = commands.find(cmd => cmd.name === process.argv[2]);
+  const isValidCommand = commands.find(cmd => cmd.name.split(" ")[0] === process.argv[2]);


This is needed because the rnpm commands have additional positional arguments, e.g link <packageName> in their names, therefore a naive full string comparison wouldn't work.

Looks like this might be breaking all usages of link ABCD with the RC? If yes, that has to be cherry-picked into stable release.

Yep. Any chance you could take this whole change?

Yeah, actually @rozele submitted a PR fixing that particular thing, mind rebasing in like an hour once the merge finishes?

Which PR? I don't think #9173 is related, it only fixes showing the help. When you run the command react-native link react-native-plugin, you would still see the warning because isValidCommand fails to find a command with a name that matches. This fixes that.

I noticed you added "needs-revision", what exactly needs revision? Also, given that this is not the main point of this PR, I would appreciate it if you could comment on the addition of the pre/post unlink hooks :).

Yeah, but we have already merged a fix for that here: https://github.com/facebook/react-native/blob/master/local-cli/cliEntry.js#L161

Kureev · 2016-08-02T07:47:16Z

Postlink already exists: https://github.com/rnpm/rnpm#commands

Kureev · 2016-08-02T07:47:55Z

Post-unlink is nothing else but something you may run after rnpm unlink: rnpm unlink <packageName> && doSomething

geof90 · 2016-08-02T07:52:18Z

@Kureev Sorry I meant preunlink and postunlink in the test plan.

Yes, we can do that, but as a user, it will be much more convenient if RNPM would call those hooks for plugin authors instead of having the user do something different for every react native plugin they want to unlink.

ghost · 2016-08-02T07:57:20Z

@geof90 updated the pull request.

rozele · 2016-08-03T12:05:34Z

local-cli/rnpm/link/link.js

@@ -1,5 +1,5 @@
 module.exports = {
  func: require('./src/link'),
  description: 'links all native dependencies',
-  name: 'link [packageName]',
+  name: 'link <packageName>',


the packageName argument should remain optional shouldn't it?

for link - yes, for unlink it's mandatory

ghost · 2016-08-03T16:18:21Z

@geof90 updated the pull request.

geof90 · 2016-08-08T23:57:41Z

Any updates on this?

grabbou · 2016-08-16T10:58:34Z

I am leaving that up to @Kureev since he's been the author of pre/post links. Mind taking a look?

Kureev · 2016-08-21T12:24:08Z

LGTM, diff is really nice ❤️ (sorry for delay, was on vacation)

Kureev · 2016-08-21T12:24:15Z

@facebook-github-bot shipit

grabbou · 2016-08-21T20:14:51Z

That won't ship since there's a conflict with master that I commented on. The split(" ") was already merged as a part of another PR so it has to be removed from that one or rebased :)

Kureev · 2016-08-21T22:42:53Z

Can you update this PR according to @grabbou's comment, @geof90?

ghost · 2016-08-22T02:53:20Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

ghost · 2016-08-22T17:54:37Z

@geof90 updated the pull request - view changes - changes since last import

geof90 · 2016-08-22T17:56:02Z

I thought I already updated to master before after you told me that :/ I just fixed the conflicts, could you try again?

ghost · 2016-08-22T18:29:27Z

@geof90 updated the pull request - view changes - changes since last import

Kureev · 2016-08-22T19:22:58Z

I'd like to, but there are some errors: https://circleci.com/gh/facebook/react-native/10404

ghost · 2016-08-22T23:19:51Z

@geof90 updated the pull request - view changes - changes since last import

geof90 · 2016-08-22T23:22:45Z

@Kureev sorry, didn't realize the folder structure was changed

Kureev · 2016-08-23T12:17:19Z

@bestander this one looks good. Seems that circle fails because of something else (Buck?). Can we ship it?

bestander · 2016-08-24T16:14:39Z

Yes, thanks for double checking

bestander · 2016-08-24T16:15:17Z

Thus Buck issue will be gone with rebase, no need to do it this time

bestander · 2016-08-24T16:15:54Z

@facebook-github-bot shipit

ghost · 2016-08-24T16:17:06Z

Thanks for importing.If you are an FB employee go to Phabricator to review internal test results.

Summary: An attempt to address facebook/react-native#9156. cc grabbou Kureev **Test plan (required)** 1. Added the following to the `package.json` of a `react-native-plugin` ``` "rnpm": { "commands": { "preunlink": "node node_modules/react-native-plugin/scripts/preunlink" "postunlink": "node node_modules/react-native-plugin/scripts/postunlink" } } ``` 2. Added files, `scripts/preunlink.js` and `scripts/postunlink.js` to the plugin. Each of them simply logs a string to the console. 3. Ran `react-native unlink react-native-plugin` to verify that those logs get printed. Closes facebook/react-native#9157 Differential Revision: D3749434 fbshipit-source-id: 40b94c9026db4f11e8f5be4a417a0670e8069be6

add pre/postunlink

024d623

ghost added the GH Review: review-needed label Aug 2, 2016

[] to <>

3a14bf8

geof90 reviewed Aug 2, 2016
View reviewed changes

geof90 changed the title ~~Add pre/postunlink~~ [RNPM] Add pre/postunlink Aug 2, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2016

use short circuit guard instead of wrapper if

c99ff55

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2016

geof90 mentioned this pull request Aug 2, 2016

Add postlink scripts microsoft/react-native-code-push#450

Merged

grabbou added GH Review: needs-revision and removed GH Review: review-needed labels Aug 3, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016

rozele reviewed Aug 3, 2016
View reviewed changes

Update link.js

c1c2fd3

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Aug 3, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 3, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 16, 2016

ghost added GH Review: accepted and removed GH Review: review-needed labels Aug 21, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 21, 2016

ghost added Import Failed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 22, 2016

update to master

6341dc8

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2016

update error message

73c1d70

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2016

move files to right subdirectory

d4f581a

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 23, 2016

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 24, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 24, 2016

ghost closed this in e7521a1 Aug 24, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNPM] Add pre/postunlink #9157

[RNPM] Add pre/postunlink #9157

geof90 commented Aug 2, 2016 •

edited

Loading

ghost commented Aug 2, 2016

geof90 Aug 2, 2016 •

edited

Loading

grabbou Aug 2, 2016

geof90 Aug 2, 2016

grabbou Aug 3, 2016

geof90 Aug 3, 2016 •

edited

Loading

grabbou Aug 16, 2016

Kureev commented Aug 2, 2016

Kureev commented Aug 2, 2016

geof90 commented Aug 2, 2016 •

edited

Loading

ghost commented Aug 2, 2016

rozele Aug 3, 2016

Kureev Aug 3, 2016

geof90 Aug 3, 2016

ghost commented Aug 3, 2016

geof90 commented Aug 8, 2016

grabbou commented Aug 16, 2016

Kureev commented Aug 21, 2016 •

edited

Loading

Kureev commented Aug 21, 2016

grabbou commented Aug 21, 2016 •

edited

Loading

Kureev commented Aug 21, 2016

ghost commented Aug 22, 2016

ghost commented Aug 22, 2016

geof90 commented Aug 22, 2016

ghost commented Aug 22, 2016

Kureev commented Aug 22, 2016

ghost commented Aug 22, 2016

geof90 commented Aug 22, 2016

Kureev commented Aug 23, 2016

bestander commented Aug 24, 2016

bestander commented Aug 24, 2016

bestander commented Aug 24, 2016

ghost commented Aug 24, 2016

[RNPM] Add pre/postunlink #9157

[RNPM] Add pre/postunlink #9157

Conversation

geof90 commented Aug 2, 2016 • edited Loading

ghost commented Aug 2, 2016

geof90 Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

grabbou Aug 2, 2016

Choose a reason for hiding this comment

geof90 Aug 2, 2016

Choose a reason for hiding this comment

grabbou Aug 3, 2016

Choose a reason for hiding this comment

geof90 Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

grabbou Aug 16, 2016

Choose a reason for hiding this comment

Kureev commented Aug 2, 2016

Kureev commented Aug 2, 2016

geof90 commented Aug 2, 2016 • edited Loading

ghost commented Aug 2, 2016

rozele Aug 3, 2016

Choose a reason for hiding this comment

Kureev Aug 3, 2016

Choose a reason for hiding this comment

geof90 Aug 3, 2016

Choose a reason for hiding this comment

ghost commented Aug 3, 2016

geof90 commented Aug 8, 2016

grabbou commented Aug 16, 2016

Kureev commented Aug 21, 2016 • edited Loading

Kureev commented Aug 21, 2016

grabbou commented Aug 21, 2016 • edited Loading

Kureev commented Aug 21, 2016

ghost commented Aug 22, 2016

ghost commented Aug 22, 2016

geof90 commented Aug 22, 2016

ghost commented Aug 22, 2016

Kureev commented Aug 22, 2016

ghost commented Aug 22, 2016

geof90 commented Aug 22, 2016

Kureev commented Aug 23, 2016

bestander commented Aug 24, 2016

bestander commented Aug 24, 2016

bestander commented Aug 24, 2016

ghost commented Aug 24, 2016

geof90 commented Aug 2, 2016 •

edited

Loading

geof90 Aug 2, 2016 •

edited

Loading

geof90 Aug 3, 2016 •

edited

Loading

geof90 commented Aug 2, 2016 •

edited

Loading

Kureev commented Aug 21, 2016 •

edited

Loading

grabbou commented Aug 21, 2016 •

edited

Loading