Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
philikon wants to merge 1 commit into from
Closed

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

philikon wants to merge 1 commit into from

Conversation

@philikon
Copy link
Contributor

@philikon philikon commented Sep 7, 2016

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

@ghost
Copy link

ghost commented Sep 7, 2016

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

@ghost 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 Sep 7, 2016
@philikon
Copy link
Contributor Author

philikon commented Sep 7, 2016

Since this fixes a regression introduced in 0.32, can we get this uplifted at least to the 0.33-stable branch?

@philikon
Copy link
Contributor Author

philikon commented Sep 7, 2016

also cc @davidaurelio for review

@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 7, 2016
@ghost 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 Sep 8, 2016
@philikon
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
@philikon philikon force-pushed the restore_transformer branch from 7a93245 to 4a5f9e6 Compare September 8, 2016 21:31
@ghost
Copy link

ghost commented Sep 8, 2016

@philikon updated the pull request - view changes

@ghost ghost added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 8, 2016
@philikon
Copy link
Contributor Author

philikon commented Sep 12, 2016

Ping?

mkonicek
mkonicek reviewed Sep 12, 2016
View reviewed changes
local-cli/bundle/bundleCommandLineArgs.js
}, {
command: '--transformer [string]',
description: 'Specify a custom transformer to be used (absolute path)',
default: require.resolve('../../packager/transformer'),
Copy link
Contributor

@mkonicek mkonicek Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable and simple that I think I can merge this. Just one question - why remove the default here? Could it break any code relying on the default?

Copy link
Contributor Author

@philikon philikon Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the default for command line arguments is pricely the point here (and the fact that #7899 added them again is the regression from #7961). We want the transformer to be specified as followed:

  1. If specified on command line, use that
  2. If specified by rn-cli.config.js, use that
  3. Fallback to default configuration

For this fallback chain to work properly, there must not be a default value returned by the command line parser, otherwise we stop at (1) and never make it to (2).

@mkonicek
Copy link
Contributor

mkonicek commented Sep 12, 2016

Ah cool, I misunderstood that this PR removes the "Fallback to default configuration". Thanks for the explanation!

@facebook-github-bot shipit

@ghost ghost 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: 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 Sep 12, 2016
@ghost
Copy link

ghost commented Sep 12, 2016

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

@facebook-github-bot 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 Sep 12, 2016
@ghost ghost closed this in 111ed8d Sep 12, 2016
@philikon philikon deleted the restore_transformer branch September 12, 2016 23:12
miklschmidt pushed a commit to secoya/react-native that referenced this pull request Oct 7, 2016
Allow rn-cli.config.js to specify the default transformer, again
5cc25e3 
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
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
@philikon
Allow rn-cli.config.js to specify the default transformer, again
c29c12a 
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 facebook/react-native#9799

Differential Revision: D3852601

Pulled By: mkonicek

fbshipit-source-id: fc3c80bdb254145fefa870eea1828b4ef33f9297
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@davidaurelio davidaurelio

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@philikon @mkonicek @davidaurelio @facebook-github-bot