Allow rn-cli.config.js to specify the default transformer #7961

philikon · 2016-06-07T01:21:02Z

This will allow consumers to supply their own transformer to all react-native cli commands by simply implementing rn-cli.config.js and overriding getTransformModulePath(). That way they don't have to fork various parts of the iOS and Android build system that React Native already provides just to add a --transformer command line argument.

Test plan: Applied this patch to the React Native version in my app, implemented getTransformModulePath() in my rn-cli.config.js, and verified that my custom transformer is invoked.

ghost · 2016-06-07T01:22:24Z

By analyzing the blame information on this pull request, we identified @sam-swarr and @martinbigio to be potential reviewers.

ide · 2016-06-07T10:12:21Z

local-cli/bundle/buildBundle.js

      getTransformOptionsModulePath: config.getTransformOptionsModulePath,
-      transformModulePath: path.resolve(args.transformer),
+      transformModulePath: path.resolve(args.transformer) ||
+        config.getTransformModulePath(),


this never gets hit

This is what I get for last minute refactoring.

ide · 2016-06-07T10:14:47Z

path.resolve(null) crashes -- see the tests

ghost · 2016-06-08T00:17:17Z

@philikon updated the pull request.

ghost · 2016-06-08T01:19:18Z

@philikon updated the pull request.

ghost · 2016-06-08T01:19:20Z

@philikon updated the pull request.

ide · 2016-06-08T07:03:18Z

@facebook-github-bot shipit

ghost · 2016-06-08T07:03:56Z

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

philikon · 2016-06-08T20:27:31Z

@ide <3 thanks!

ghost · 2016-06-08T20:48:11Z

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.

philikon · 2016-06-10T04:05:43Z

@mkonicek some help please?

philikon · 2016-06-17T04:08:00Z

Hey folks, can I just get a stack trace or some other hint for why it's failing on the FB side?

This will allow consumers to supply their own transformer to all `react-native` cli commands by simply implementing `rn-cli.config.js` and overriding `getTransformModulePath()`. That way they don't have to fork various parts of the iOS and Android build system that React Native already provides just to add a `--transformer` command line argument.

mkonicek · 2016-06-17T15:58:16Z

@philikon I pinged the RN oncall. For some reason it throws when used from the internal CLI:

TypeError: config.getTransformModulePath is not a function
    at _dependencies (react-native-github/local-cli/dependencies/dependencies.js:57:90)
    at react-native-github/local-cli/dependencies/dependencies.js:23:5
    at tryCallTwo (react-native-github/node_modules/promise/lib/core.js:45:5)
    at doResolve (react-native-github/node_modules/promise/lib/core.js:200:13)
    at new Promise (react-native-github/node_modules/promise/lib/core.js:66:3)
    at dependencies (react-native-github/local-cli/dependencies/dependencies.js:22:10)
    at Object.run (react-native-internal-cli/src/cli.js:36:10)

mkonicek · 2016-06-20T13:14:57Z

Assigning to @foghina so it shows up in the internal diff queue, if you're swamped can you assign it to Andrei who's the next oncall?

mkonicek · 2016-06-20T13:15:48Z

@philikon Is it a breaking change? Does the stack trace make sense? Should we fix our internal code to work with this?

philikon · 2016-06-20T17:25:22Z

Thanks @mkonicek!

It shouldn't be a breaking change because I'm providing a fallback in local-cli/default.config.js. I suspect that react-native-internal-cli provides its own default.config.js that doesn't have the fallback, so that's why it's failing.

It'd be great if the default config in react-native-internal-cli were updated, or even better, react-native-internal-cli would fallback to react-native/local-cli/default.config.js, so that something like this doesn't happen in the future.

philikon · 2016-06-21T21:16:00Z

@foghina let me know if I can help in any way besides my comment above. Either way, I'll buy you many beers next time I'm in London :)

foghina · 2016-06-22T14:33:46Z

@philikon I think I fixed it internally. It's landing now, let's see what happens...

philikon · 2016-06-22T18:44:21Z

<3 <3 <3 @foghina

Summary: This will allow consumers to supply their own transformer to all `react-native` cli commands by simply implementing `rn-cli.config.js` and overriding `getTransformModulePath()`. That way they don't have to fork various parts of the iOS and Android build system that React Native already provides just to add a `--transformer` command line argument. **Test plan:** Applied this patch to the React Native version in my app, implemented `getTransformModulePath()` in my `rn-cli.config.js`, and verified that my custom transformer is invoked. Closes facebook#7961 Differential Revision: D3404201 Pulled By: foghina fbshipit-source-id: c7eaa85de84d485d06d23a2ffea899821b2cf71c

Summary: This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.). It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box. <s>This is still WIP and some of the commands are left commented out.</s> For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e Closes #7899 Differential Revision: D3613193 Pulled By: bestander fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037

Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899

Summary: Restores feature introduced in #7961 after it's been paved partially by #7899 **Test Plan:** ran example in https://github.com/philikon/ReactNativify against a React Native with and without this patch Closes #9799 Differential Revision: D3852601 Pulled By: mkonicek fbshipit-source-id: fc3c80bdb254145fefa870eea1828b4ef33f9297

Summary: Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899 **Test Plan:** ran example in https://github.com/philikon/ReactNativify against a React Native with and without this patch Closes facebook#9799 Differential Revision: D3852601 Pulled By: mkonicek fbshipit-source-id: fc3c80bdb254145fefa870eea1828b4ef33f9297

vikeri · 2016-10-20T14:42:36Z

@philikon What is the use case of this? Is it something that would be useful for us developing React Native with ClojureScript? (Where the Clojurescript compiler already outputs ES3 compliant code).

Summary: This will allow consumers to supply their own transformer to all `react-native` cli commands by simply implementing `rn-cli.config.js` and overriding `getTransformModulePath()`. That way they don't have to fork various parts of the iOS and Android build system that React Native already provides just to add a `--transformer` command line argument. **Test plan:** Applied this patch to the React Native version in my app, implemented `getTransformModulePath()` in my `rn-cli.config.js`, and verified that my custom transformer is invoked. Closes facebook/react-native#7961 Differential Revision: D3404201 Pulled By: foghina fbshipit-source-id: c7eaa85de84d485d06d23a2ffea899821b2cf71c

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 7, 2016

ide reviewed Jun 7, 2016
View reviewed changes

ide added GH Review: needs-revision and removed GH Review: review-needed labels Jun 7, 2016

philikon force-pushed the transformer_config branch from 29fd85d to 7266a5b Compare June 8, 2016 00:15

ghost added GH Review: review-needed and removed GH Review: needs-revision labels Jun 8, 2016

philikon force-pushed the transformer_config branch from 7266a5b to 1939e98 Compare June 8, 2016 01:18

ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jun 8, 2016

ghost added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jun 8, 2016

philikon force-pushed the transformer_config branch from 1939e98 to c44e449 Compare June 17, 2016 04:10

mkonicek assigned foghina Jun 20, 2016

ghost closed this in dd9b3e1 Jun 22, 2016

philikon deleted the transformer_config branch June 22, 2016 18:44

philikon added a commit to philikon/react-native that referenced this pull request Sep 7, 2016

Allow rn-cli.config.js to specify the default transformer, again

7a93245

Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899

philikon mentioned this pull request Sep 7, 2016

Allow rn-cli.config.js to specify the default transformer, again #9799

Closed

philikon added a commit to philikon/react-native that referenced this pull request Sep 8, 2016

Allow rn-cli.config.js to specify the default transformer, again

4a5f9e6

Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899

orta mentioned this pull request Jan 10, 2017

React Packager does not recognize *.ts files orta/systems-theory#1

Open

This pull request was closed.

Allow rn-cli.config.js to specify the default transformer #7961

Allow rn-cli.config.js to specify the default transformer #7961

Uh oh!

Conversation

philikon commented Jun 7, 2016

Uh oh!

ghost commented Jun 7, 2016

Uh oh!

ide Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

philikon Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

ide commented Jun 7, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

ide commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

philikon commented Jun 8, 2016

Uh oh!

ghost commented Jun 8, 2016

Uh oh!

philikon commented Jun 10, 2016

Uh oh!

philikon commented Jun 17, 2016

Uh oh!

mkonicek commented Jun 17, 2016

Uh oh!

mkonicek commented Jun 20, 2016

Uh oh!

mkonicek commented Jun 20, 2016

Uh oh!

philikon commented Jun 20, 2016

Uh oh!

philikon commented Jun 21, 2016

Uh oh!

foghina commented Jun 22, 2016

Uh oh!

philikon commented Jun 22, 2016

Uh oh!

vikeri commented Oct 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants