Merge rnpm into react-native #7550

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants
@grabbou
Collaborator

grabbou commented May 13, 2016

This is initial (first step) in the merging process. For now, we are just going to move our code as is into local-cli folder (first commit). There were other tweaks made in separate commits to make it easier to go through the code as the diff is expected to be rather large. The purpose of this is to make it easier to start working in small batches and improving the CLI incrementally on a daily basis.

Current codebase will still leave in rnpm organisation on Github where we keep working on new features, bugs and ship releases to npm until we finish our integration and provide a nice interface for users to migrate (in case it changes at all)

Flow, Jest and npm will ignore this folder for now until we integrate it properly.

Tests are to be rewritten from mocha to jest in rnpm/link. We will hook them all up as soon as we start using them in local-cli.

For now, there's no point in having them running and possibly breaking the builds.

We will announce next steps with @Kureev later this week.

CC: @mkonicek

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 13, 2016

By analyzing the blame information on this pull request, we identified @cpojer and @jeffmo to be potential reviewers.

ghost commented May 13, 2016

By analyzing the blame information on this pull request, we identified @cpojer and @jeffmo to be potential reviewers.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 13, 2016

@grabbou updated the pull request.

ghost commented May 13, 2016

@grabbou updated the pull request.

@Kureev

This comment has been minimized.

Show comment
Hide comment
@Kureev

Kureev May 15, 2016

Contributor

Thanks for taking it over, @grabbou ❤️

Contributor

Kureev commented May 15, 2016

Thanks for taking it over, @grabbou ❤️

@joewood joewood referenced this pull request in Microsoft/react-native-windows May 16, 2016

Closed

Roadmap For Missing Features and Plans to Merge? #422

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide May 17, 2016

Collaborator

I assume you guys considered publishing rnpm to npm and installing it from there (react-native's package.json would depend on rnpm). That seems like an almost strictly better idea than merging it into this repo but maybe there are good reasons not to?

Collaborator

ide commented May 17, 2016

I assume you guys considered publishing rnpm to npm and installing it from there (react-native's package.json would depend on rnpm). That seems like an almost strictly better idea than merging it into this repo but maybe there are good reasons not to?

@Kureev

This comment has been minimized.

Show comment
Hide comment
@Kureev

Kureev May 18, 2016

Contributor

I think @mkonicek wants to see rnpm as a part of react-native (merged into core).

Contributor

Kureev commented May 18, 2016

I think @mkonicek wants to see rnpm as a part of react-native (merged into core).

@grabbou

This comment has been minimized.

Show comment
Hide comment
@grabbou

grabbou May 18, 2016

Collaborator

but maybe there are good reasons not to?

The main reason was that hopefully in like 2 or 3 months there will be no rnpm and everything is going to be merged / added / implemented as a part of local-cli. That includes things like upgrade command or these detection mechanism. Being in the repo means we can optimise the releases better and gain more contributions / code reviews w/o working on it alone.

We can of course require it normally from the react-native or just move out entire cli out of react-native repo and make the rnpm peerDependency (that has the benefit of not having the yeoman installed here).

Collaborator

grabbou commented May 18, 2016

but maybe there are good reasons not to?

The main reason was that hopefully in like 2 or 3 months there will be no rnpm and everything is going to be merged / added / implemented as a part of local-cli. That includes things like upgrade command or these detection mechanism. Being in the repo means we can optimise the releases better and gain more contributions / code reviews w/o working on it alone.

We can of course require it normally from the react-native or just move out entire cli out of react-native repo and make the rnpm peerDependency (that has the benefit of not having the yeoman installed here).

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek May 18, 2016

Contributor

@ide I thought since features like rnpm link are such core to React Native it made sense to make it part of React Native itself. Adding a dependency in package.json would work too. Since rnpm is going to be part of the CLI (react-native link for example) it might be easier to have the CLI and rnpm in the same repo, to be able to refactor code etc. This will also make rnpm part of the release. I remember with other parts of React Native, such as node-haste we had some issues with them living in a separate repo, such as not everyone working on React Native had a permission to npm publish when we needed to publish a fix. Is there an advantage of using a separate repo?

Contributor

mkonicek commented May 18, 2016

@ide I thought since features like rnpm link are such core to React Native it made sense to make it part of React Native itself. Adding a dependency in package.json would work too. Since rnpm is going to be part of the CLI (react-native link for example) it might be easier to have the CLI and rnpm in the same repo, to be able to refactor code etc. This will also make rnpm part of the release. I remember with other parts of React Native, such as node-haste we had some issues with them living in a separate repo, such as not everyone working on React Native had a permission to npm publish when we needed to publish a fix. Is there an advantage of using a separate repo?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek May 18, 2016

Contributor

Being in the repo means we can optimise the releases better and gain more contributions / code reviews w/o working on it alone.

This is a good argument too.

Contributor

mkonicek commented May 18, 2016

Being in the repo means we can optimise the releases better and gain more contributions / code reviews w/o working on it alone.

This is a good argument too.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek May 20, 2016

Contributor

Tests passed, let's shipit :)

@facebook-github-bot shipit

Contributor

mkonicek commented May 20, 2016

Tests passed, let's shipit :)

@facebook-github-bot shipit

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 20, 2016

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

ghost commented May 20, 2016

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

@ghost ghost closed this in 149d0b9 May 20, 2016

zebulgar added a commit to nightingale/react-native that referenced this pull request Jun 18, 2016

Merge rnpm into react-native
Summary:
This is initial (first step) in the merging process. For now, we are just going to move our code as is into `local-cli` folder (first commit). There were other tweaks made in separate commits to make it easier to go through the code as the diff is expected to be rather large. The purpose of this is to make it easier to start working in small batches and improving the CLI incrementally on a daily basis.

Current codebase will still leave in `rnpm` organisation on Github where we keep working on new features, bugs and ship releases to `npm` until we finish our integration and provide a nice interface for users to migrate (in case it changes at all)

Flow, Jest and npm will ignore this folder for now until we integrate it properly.

Tests are to be rewritten from mocha to jest in `rnpm/link`. We will hook them all up as soon as we start using them in local-cli.

For now, there's no point in having them running and possibly breaking the builds.

We will announce next steps with Kureev later this week
Closes facebook#7550

Differential Revision: D3327772

Pulled By: mkonicek

fbshipit-source-id: 90faa4bd78476d93ed21b1253e0d95c755d28a30

samerce added a commit to iodine/react-native that referenced this pull request Aug 23, 2016

Merge rnpm into react-native
Summary:
This is initial (first step) in the merging process. For now, we are just going to move our code as is into `local-cli` folder (first commit). There were other tweaks made in separate commits to make it easier to go through the code as the diff is expected to be rather large. The purpose of this is to make it easier to start working in small batches and improving the CLI incrementally on a daily basis.

Current codebase will still leave in `rnpm` organisation on Github where we keep working on new features, bugs and ship releases to `npm` until we finish our integration and provide a nice interface for users to migrate (in case it changes at all)

Flow, Jest and npm will ignore this folder for now until we integrate it properly.

Tests are to be rewritten from mocha to jest in `rnpm/link`. We will hook them all up as soon as we start using them in local-cli.

For now, there's no point in having them running and possibly breaking the builds.

We will announce next steps with Kureev later this week
Closes facebook#7550

Differential Revision: D3327772

Pulled By: mkonicek

fbshipit-source-id: 90faa4bd78476d93ed21b1253e0d95c755d28a30

mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016

Merge rnpm into react-native
Summary:
This is initial (first step) in the merging process. For now, we are just going to move our code as is into `local-cli` folder (first commit). There were other tweaks made in separate commits to make it easier to go through the code as the diff is expected to be rather large. The purpose of this is to make it easier to start working in small batches and improving the CLI incrementally on a daily basis.

Current codebase will still leave in `rnpm` organisation on Github where we keep working on new features, bugs and ship releases to `npm` until we finish our integration and provide a nice interface for users to migrate (in case it changes at all)

Flow, Jest and npm will ignore this folder for now until we integrate it properly.

Tests are to be rewritten from mocha to jest in `rnpm/link`. We will hook them all up as soon as we start using them in local-cli.

For now, there's no point in having them running and possibly breaking the builds.

We will announce next steps with Kureev later this week
Closes facebook#7550

Differential Revision: D3327772

Pulled By: mkonicek

fbshipit-source-id: 90faa4bd78476d93ed21b1253e0d95c755d28a30

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment