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.

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

Enable custom source file extensions through cli args or rn-cli.config.js #13689

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
@ds300

ds300 commented Apr 27, 2017

Ping @cpojer - This is the replacement PR for #11932

Motivation (required)

Allow the packager to load files from other compile-to-JS languages without developers having to specify file extensions in require calls.

Approach

Add an optional getSourceExts method to rn-cli.config.js, like the existing getAssetExts. Set defaults of 'js' and 'json' (as were being used before) and make sure the options are propagated through to the packager appropriately, then use them in module resolution.

Extensions specified by getSourceExts do not override the defaults but are combined with them, otherwise first-party react-native code would not be loaded.

Test Plan (required)

Tests for the changes to node-haste are included. I couldn't find any integration tests for rn-cli.config.js or the cli config opts anywhere, so I figured it wouldn't be necessary to add those. If that is required, it would be dope if someone with the appropriate knowledge could help direct that effort.

Here is a repo that shows it working: https://github.com/ds300/TestCustomSourceExts

Cheers

ds300 added some commits Apr 27, 2017

add failing test for custom source extensions feature
test fails with this error:

    UnableToResolveError: Unable to resolve module `./a` from `/root/index.jsx`: Directory /root/a doesn't exist

      at UnableToResolveError (packager/src/node-haste/DependencyGraph/ResolutionRequest.js:625:5)
      at ResolutionRequest._loadAsDir (packager/src/node-haste/DependencyGraph/ResolutionRequest.js:585:13)
      at tryResolveSync (packager/src/node-haste/DependencyGraph/ResolutionRequest.js:441:16)
      at tryResolveSync (packager/src/node-haste/DependencyGraph/ResolutionRequest.js:88:12)
      ...
@ds300

This comment has been minimized.

Show comment
Hide comment
@ds300

ds300 Apr 28, 2017

I updated the TestCustomSourceExts repo to have incremental commits so people can see exactly what changed from a regular old blank react-native app.

ds300 commented Apr 28, 2017

I updated the TestCustomSourceExts repo to have incremental commits so people can see exactly what changed from a regular old blank react-native app.

@cpojer cpojer requested a review from jeanlauliac Apr 28, 2017

@jeanlauliac

Looks good to me! The "default config" system is a bit of a mess, for example packager/defaults really is the "base", not overridable defaults. We'll look into fixing that later on. This changeset follows the same way assetExts had been done so that's good.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Apr 28, 2017

@jeanlauliac has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented Apr 28, 2017

@jeanlauliac has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jeanlauliac

There's been a bunch of changes happening, I'm rebasing this on the latest and doing a few tweaks.

@jeanlauliac

I'm just putting back it in 'change requested' state while waiting for the rebase, but I'm dealing with it and there's no action needed on your part right now.

@jeanlauliac

This comment has been minimized.

Show comment
Hide comment
@jeanlauliac

jeanlauliac May 3, 2017

Contributor

I'll ship that tomorrow morning London-time.

Contributor

jeanlauliac commented May 3, 2017

I'll ship that tomorrow morning London-time.

@jeanlauliac

This comment has been minimized.

Show comment
Hide comment
@jeanlauliac

jeanlauliac May 4, 2017

Contributor

4a86f93 is the upgraded version I landed. @ds300 Do you want to check it out in https://github.com/ds300/TestCustomSourceExts and verify it covers what you need?

Contributor

jeanlauliac commented May 4, 2017

4a86f93 is the upgraded version I landed. @ds300 Do you want to check it out in https://github.com/ds300/TestCustomSourceExts and verify it covers what you need?

@jeanlauliac jeanlauliac self-assigned this May 4, 2017

@ds300

This comment has been minimized.

Show comment
Hide comment
@ds300

ds300 May 4, 2017

@jeanlauliac I'm getting this error after switching to your branch. The cause wasn't obvious from initial inspection.

Error: Cannot find module '/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/rn-cli.config.js'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at Object.loadFile (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/util/Config.js:137:24)
    at getCliConfig (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/index.js:44:14)
    at Object.<anonymous> (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/index.js:50:18)
    at Module._compile (module.js:541:32)
    at loader (/Users/dshe/code/TestCustomSourceExts/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/dshe/code/TestCustomSourceExts/node_modules/babel-register/lib/node.js:154:7)

Your changes look sensible to me otherwise. This is probably related to other changes that have happened in the meantime. I'll try to take a proper look on Sunday evening UK time.

ds300 commented May 4, 2017

@jeanlauliac I'm getting this error after switching to your branch. The cause wasn't obvious from initial inspection.

Error: Cannot find module '/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/rn-cli.config.js'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at Object.loadFile (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/util/Config.js:137:24)
    at getCliConfig (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/index.js:44:14)
    at Object.<anonymous> (/Users/dshe/code/TestCustomSourceExts/node_modules/react-native/local-cli/core/index.js:50:18)
    at Module._compile (module.js:541:32)
    at loader (/Users/dshe/code/TestCustomSourceExts/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/dshe/code/TestCustomSourceExts/node_modules/babel-register/lib/node.js:154:7)

Your changes look sensible to me otherwise. This is probably related to other changes that have happened in the meantime. I'll try to take a proper look on Sunday evening UK time.

@jmfirth

This comment has been minimized.

Show comment
Hide comment
@jmfirth

jmfirth May 5, 2017

I've been eager to use this for a while and have been kicking the tires a bit.

With a yarn install the iOS app works and the Android app does not. To resolve this, I reverted to react-native@0.44.0 to build the Android shell and then restored the patch.

With this environment everything seems to work great except for source maps -- they are off by a few lines.

image
(Breakpoint should be on debugger;)

jmfirth commented May 5, 2017

I've been eager to use this for a while and have been kicking the tires a bit.

With a yarn install the iOS app works and the Android app does not. To resolve this, I reverted to react-native@0.44.0 to build the Android shell and then restored the patch.

With this environment everything seems to work great except for source maps -- they are off by a few lines.

image
(Breakpoint should be on debugger;)

@ds300

This comment has been minimized.

Show comment
Hide comment
@ds300

ds300 May 6, 2017

@jmfirth I think that's an orthogonal issue. This PR doesn't say anything about how source transformation should happen. And in particular the transformer I wrote for the test repo is extremely simplistic and doesn't compose the tsc source map with the babel one. So if you are using that, the discrepancy you observe makes sense.

ds300 commented May 6, 2017

@jmfirth I think that's an orthogonal issue. This PR doesn't say anything about how source transformation should happen. And in particular the transformer I wrote for the test repo is extremely simplistic and doesn't compose the tsc source map with the babel one. So if you are using that, the discrepancy you observe makes sense.

@ds300

This comment has been minimized.

Show comment
Hide comment
@ds300

ds300 May 7, 2017

@jeanlauliac the issue was obvious after all. There has been a breaking change in handling the --config cli option: https://github.com/facebook/react-native/blame/4a86f93982ede66a993a72e31cadb85d9bd4b3a3/local-cli/core/index.js#L44

Now the path given to --config must be absolute, or else it is evaluated against the __dirname of local-cli/core/index.js.

cc @davidaurelio this seems like a bad design decision to me, since it makes it super awkward to have a config file in a multi-dev project, unless I'm missing something.

But also that's beside the point for this PR. Making the path absolute makes the problem go away, and now I get a new error, but I also get that same error if I run a blank RN project against master, so it is likely to be unrelated.

In other words, I'm happy with your changes @jeanlauliac 👍

ds300 commented May 7, 2017

@jeanlauliac the issue was obvious after all. There has been a breaking change in handling the --config cli option: https://github.com/facebook/react-native/blame/4a86f93982ede66a993a72e31cadb85d9bd4b3a3/local-cli/core/index.js#L44

Now the path given to --config must be absolute, or else it is evaluated against the __dirname of local-cli/core/index.js.

cc @davidaurelio this seems like a bad design decision to me, since it makes it super awkward to have a config file in a multi-dev project, unless I'm missing something.

But also that's beside the point for this PR. Making the path absolute makes the problem go away, and now I get a new error, but I also get that same error if I run a blank RN project against master, so it is likely to be unrelated.

In other words, I'm happy with your changes @jeanlauliac 👍

@davidaurelio

This comment has been minimized.

Show comment
Hide comment
@davidaurelio

davidaurelio May 8, 2017

Contributor

Hey @ds300, that was the case before my commit as well. I agree it is awkward. Correct me if I am wrong, but I think my changeset did not change behavior at all:

before, both config file and __dirname were passed to Config.loadFile, and that function would call path.join if the config file was not an absolute path. Now, we call path.resolve with both relative and absolute paths at the call site, which should have the same behaviour, right?

Contributor

davidaurelio commented May 8, 2017

Hey @ds300, that was the case before my commit as well. I agree it is awkward. Correct me if I am wrong, but I think my changeset did not change behavior at all:

before, both config file and __dirname were passed to Config.loadFile, and that function would call path.join if the config file was not an absolute path. Now, we call path.resolve with both relative and absolute paths at the call site, which should have the same behaviour, right?

@davidaurelio

This comment has been minimized.

Show comment
Hide comment
@davidaurelio

davidaurelio May 8, 2017

Contributor

Feel free to send a PR that changes the base of the resolution to process.cwd(). That seems a lot better.

Contributor

davidaurelio commented May 8, 2017

Feel free to send a PR that changes the base of the resolution to process.cwd(). That seems a lot better.

@ds300

This comment has been minimized.

Show comment
Hide comment
@ds300

ds300 May 8, 2017

Oh, true, @davidaurelio my apologies. I didn't look too closely at the history. Will investigate tonight to find out exactly when support for relative paths was removed and why.

ds300 commented May 8, 2017

Oh, true, @davidaurelio my apologies. I didn't look too closely at the history. Will investigate tonight to find out exactly when support for relative paths was removed and why.

@jeanlauliac

This comment has been minimized.

Show comment
Hide comment
@jeanlauliac

jeanlauliac May 9, 2017

Contributor

Closing, let's open separate tasks or PRs to address the other issues.

On an unrelated note, I'm disappointed in myself that I didn't properly give you credit for the changeset. Even though I rebased and changed a few things, you provided most of the idea and original code. I will be careful to maintain a correct Authored-by field next time I merge a PR. I regret not doing so.

Contributor

jeanlauliac commented May 9, 2017

Closing, let's open separate tasks or PRs to address the other issues.

On an unrelated note, I'm disappointed in myself that I didn't properly give you credit for the changeset. Even though I rebased and changed a few things, you provided most of the idea and original code. I will be careful to maintain a correct Authored-by field next time I merge a PR. I regret not doing so.

@jeanlauliac jeanlauliac closed this May 9, 2017

@orta orta referenced this pull request May 23, 2017

Closed

Update React Native to 0.45 #560

@sarahscott sarahscott referenced this pull request May 26, 2017

Merged

[Dev] RN v0.45 Upgrade #570

10 of 10 tasks complete
@johnnycopperstone

This comment has been minimized.

Show comment
Hide comment
@johnnycopperstone

johnnycopperstone Jun 12, 2017

Slightly confused with what's happening with this PR. Has it been released with 0.45 ?

johnnycopperstone commented Jun 12, 2017

Slightly confused with what's happening with this PR. Has it been released with 0.45 ?

@ds300

This comment has been minimized.

Show comment
Hide comment

ds300 commented Jun 12, 2017

@johnnycopperstone yep.

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