Skip to content

feat: support local RN libraries in autolinking #451

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

Merged
merged 3 commits into from
Jul 2, 2019
Merged

feat: support local RN libraries in autolinking #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2636a2a
feat: support local RN libraries in autolinking
thymikee Jun 30, 2019
beb7c0c
simplify local deps config
thymikee Jun 30, 2019
9b3a67d
address feedback
thymikee Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion docs/autolinking.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,12 @@ On the iOS side, you will need to ensure you have a Podspec to the root of your

A library can add a `react-native.config.js` configuration file, which will customize the defaults.

## How do I disable autolinking for unsupported package?
## How can I disable autolinking for unsupported library?

During the transition period some packages may not support autolinking on certain platforms. To disable autolinking for a package, update your `react-native.config.js`'s `dependencies` entry to look like this:

```js
// react-native.config.js
module.exports = {
dependencies: {
'some-unsupported-package': {
Expand All @@ -109,3 +110,18 @@ module.exports = {
},
};
```

## How can I autolink a local library?

We can leverage CLI configuration to make it "see" React Native libraries that are not part of our 3rd party dependencies. To do so, update your `react-native.config.js`'s `dependencies` entry to look like this:

```js
// react-native.config.js
module.exports = {
dependencies: {
'local-rn-library': {
root: '/root/libraries',
},
},
};
```
14 changes: 13 additions & 1 deletion docs/projects.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ For example, you could set:
```js
module.exports = {
dependencies: {
['react-native-webview']: {
'react-native-webview': {
platforms: {
ios: null,
},
Expand All @@ -120,6 +120,18 @@ module.exports = {

in order to disable linking of React Native WebView on iOS.

Another use-case would be supporting local libraries that are not discoverable for autolinking, since they're not part of your `dependencies` or `devDependencies`:

```js
module.exports = {
dependencies: {
'local-rn-library': {
root: '/root/libraries',
},
},
};
```

The object provided here is deep merged with the dependency config. Check [`projectConfig`](platforms.md#projectconfig) and [`dependencyConfig`](platforms.md#dependencyConfig) return values for a full list of properties that you can override.

> Note: This is an advanced feature and you should not need to use it mos of the time.
Expand Down
52 changes: 50 additions & 2 deletions packages/cli/src/tools/config/__tests__/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,6 @@ test.skip('should skip packages that have invalid configuration', () => {
});

test('does not use restricted "react-native" key to resolve config from package.json', () => {
jest.resetModules();

writeFiles(DIR, {
'node_modules/react-native-netinfo/package.json': `{
"react-native": "src/index.js"
Expand All @@ -302,3 +300,53 @@ test('does not use restricted "react-native" key to resolve config from package.
expect(dependencies).toHaveProperty('react-native-netinfo');
expect(spy).not.toHaveBeenCalled();
});

test('supports dependencies from user configuration with custom root and properties', () => {
writeFiles(DIR, {
'node_modules/react-native/package.json': '{}',
'native-libs/local-lib/ios/LocalRNLibrary.xcodeproj/project.pbxproj': '',
'react-native.config.js': `module.exports = {
dependencies: {
'local-lib': {
root: "${DIR}/native-libs/local-lib",
platforms: {
ios: {
podspecPath: "custom-path"
}
}
},
}
}`,
'package.json': `{
"dependencies": {
"react-native": "0.0.1"
}
}`,
});

const {dependencies} = loadConfig(DIR);
expect(removeString(dependencies['local-lib'], DIR)).toMatchInlineSnapshot(`
Object {
"assets": Array [],
"hooks": Object {},
"name": "local-lib",
"params": Array [],
"platforms": Object {
"android": null,
"ios": Object {
"folder": "<<REPLACED>>/native-libs/local-lib",
"libraryFolder": "Libraries",
"pbxprojPath": "<<REPLACED>>/native-libs/local-lib/ios/LocalRNLibrary.xcodeproj/project.pbxproj",
"plist": Array [],
"podfile": null,
"podspecPath": "custom-path",
"projectName": "LocalRNLibrary.xcodeproj",
"projectPath": "<<REPLACED>>/native-libs/local-lib/ios/LocalRNLibrary.xcodeproj",
"sharedLibraries": Array [],
"sourceDir": "<<REPLACED>>/native-libs/local-lib/ios",
},
},
"root": "<<REPLACED>>/native-libs/local-lib",
}
`);
});
158 changes: 81 additions & 77 deletions packages/cli/src/tools/config/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function loadConfig(projectRoot: string = process.cwd()): ConfigT {
? path.resolve(projectRoot, userConfig.reactNativePath)
: resolveReactNativePath(projectRoot);
},
dependencies: {},
dependencies: userConfig.dependencies,
commands: userConfig.commands,
get assets() {
return findAssets(projectRoot, userConfig.assets);
Expand All @@ -97,90 +97,94 @@ function loadConfig(projectRoot: string = process.cwd()): ConfigT {

let depsWithWarnings = [];

const finalConfig = findDependencies(projectRoot).reduce(
(acc: ConfigT, dependencyName) => {
let root;
let config;
try {
root = resolveNodeModuleDir(projectRoot, dependencyName);
const output = readDependencyConfigFromDisk(root);
config = output.config;
const finalConfig = [
...Object.keys(userConfig.dependencies),
...findDependencies(projectRoot),
].reduce((acc: ConfigT, dependencyName) => {
const localDependencyRoot =
userConfig.dependencies[dependencyName] &&
userConfig.dependencies[dependencyName].root;
Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

This code has a potential side effect of users defining react-native-camera.root and being able to overwrite the root for that library. I don't think this is intentional and can be confusing.

Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

There's also one conceptual thing regarding userConfig that I really wanted to avoid. By default, user configuration is deep merged with the configuration we find out. While finding out the proper configuration, we take dependency config into consideration.

User provided configuration can overwrite any part of the dependency configuration but that will not affect other properties. For example, you can overwrite the android package import path, but this will not affect other properties.

Taking root into consideration breaks this contract.

Also, have we tested for duplicate keys?

Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

What if we exported a utility function instead and asked users to do:

module.exports = {
   dependencies: {
      'my-custom-dependency': resolveDependencyINPath('...')
   },
};

that would still preserve the merging approach we have settled on so far and actually, give us an opportunity to extract some pieces of this file into reusable functions?

Copy link
Member Author

@thymikee thymikee Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

User provided configuration can overwrite any part of the dependency configuration but that will not affect other properties. For example, you can overwrite the android package import path, but this will not affect other properties.
Taking root into consideration breaks this contract

Not really sure about that. root is kinda special, because it lets CLI resolve other values, but you can still override them:

const root = process.cwd();

module.exports = {
  dependencies: {
    'local-lib': {
      root,
      platforms: {
        ios: {
          sourceDir: 'lala',
          libraryFolder: 'lol/lib',
        },
      },
    },
  },
};

resolves to:

{
  "dependencies": {
    "local-lib": {
      "root": "/some-root",
      "name": "local-lib",
      "platforms": {
        "ios": {
          "sourceDir": "lala",
          // regular params
          "libraryFolder": "lol/lib",
        },
        "android": {
          // regular params
        }
      },
      "assets": [],
      "hooks": {},
      "params": []
    }
  }
}

That's why I chose this solution. It's "nothing new" and user is still able to override things.

Copy link
Member Author

@thymikee thymikee Jul 2, 2019

Choose a reason for hiding this comment

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

I included above in a test so it doesn't go away

let root;
let config;
try {
root =
localDependencyRoot ||
resolveNodeModuleDir(projectRoot, dependencyName);
const output = readDependencyConfigFromDisk(root);
config = output.config;

if (output.legacy) {
const pkg = require(path.join(root, 'package.json'));
const link =
pkg.homepage || `https://npmjs.com/package/${dependencyName}`;
depsWithWarnings.push([dependencyName, link]);
}
} catch (error) {
logger.warn(
inlineString(`
Package ${chalk.bold(
dependencyName,
)} has been ignored because it contains invalid configuration.

Reason: ${chalk.dim(error.message)}
`),
);
return acc;
if (output.legacy && !localDependencyRoot) {
Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

Why && !localDependencyRoot has been added here?

Copy link
Member Author

@thymikee thymikee Jul 1, 2019

Choose a reason for hiding this comment

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

because local deps don't have package.json

const pkg = require(path.join(root, 'package.json'));
const link =
pkg.homepage || `https://npmjs.com/package/${dependencyName}`;
depsWithWarnings.push([dependencyName, link]);
}
} catch (error) {
logger.warn(
inlineString(`
Package ${chalk.bold(
dependencyName,
)} has been ignored because it contains invalid configuration.

/**
* @todo: remove this code once `react-native` is published with
* `platforms` and `commands` inside `react-native.config.js`.
*/
if (dependencyName === 'react-native') {
if (Object.keys(config.platforms).length === 0) {
config.platforms = {ios, android};
}
if (config.commands.length === 0) {
config.commands = [...ios.commands, ...android.commands];
}
Reason: ${chalk.dim(error.message)}`),
);
return acc;
}

/**
* @todo: remove this code once `react-native` is published with
* `platforms` and `commands` inside `react-native.config.js`.
*/
if (dependencyName === 'react-native') {
if (Object.keys(config.platforms).length === 0) {
config.platforms = {ios, android};
}
if (config.commands.length === 0) {
config.commands = [...ios.commands, ...android.commands];
}
}

const isPlatform = Object.keys(config.platforms).length > 0;
const isPlatform = Object.keys(config.platforms).length > 0;

/**
* Legacy `rnpm` config required `haste` to be defined. With new config,
* we do it automatically.
*
* @todo: Remove this once `rnpm` config is deprecated and all major RN libs are converted.
*/
const haste = config.haste || {
providesModuleNodeModules: isPlatform ? [dependencyName] : [],
platforms: Object.keys(config.platforms),
};
/**
* Legacy `rnpm` config required `haste` to be defined. With new config,
* we do it automatically.
*
* @todo: Remove this once `rnpm` config is deprecated and all major RN libs are converted.
*/
const haste = config.haste || {
providesModuleNodeModules: isPlatform ? [dependencyName] : [],
platforms: Object.keys(config.platforms),
};

return (assign({}, acc, {
dependencies: assign({}, acc.dependencies, {
// $FlowExpectedError: Dynamic getters are not supported
get [dependencyName]() {
return getDependencyConfig(
root,
dependencyName,
finalConfig,
config,
userConfig,
isPlatform,
);
},
}),
commands: [...acc.commands, ...config.commands],
platforms: {
...acc.platforms,
...config.platforms,
return (assign({}, acc, {
dependencies: assign({}, acc.dependencies, {
// $FlowExpectedError: Dynamic getters are not supported
get [dependencyName]() {
return getDependencyConfig(
Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

I know I am picky, but I actually missed this in the previous PR review. I find extracting getDependencyConfig a bit confusing as the arguments are confusing in my opinion.

0fcf6d3#diff-ab87179036e0c2b91acc179cd458e87aR21

Copy link
Member

@grabbou grabbou Jul 1, 2019

Choose a reason for hiding this comment

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

(especially the finalConfig bit from within a function is no longer clear - I think here, it made more sense because of the visible reduce and acc usage).

Copy link
Member Author

@thymikee thymikee Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But that's how it is, the finalConfig is not there, unless the whole reduce pass is done. Extracting this helped me to better understand this piece of code and actually type it properly, so I'd leave it even for the type safety sake only. At least in this PR

root,
dependencyName,
finalConfig,
config,
userConfig,
isPlatform,
);
},
haste: {
providesModuleNodeModules: [
...acc.haste.providesModuleNodeModules,
...haste.providesModuleNodeModules,
],
platforms: [...acc.haste.platforms, ...haste.platforms],
},
}): ConfigT);
},
initialConfig,
);
}),
commands: [...acc.commands, ...config.commands],
platforms: {
...acc.platforms,
...config.platforms,
},
haste: {
providesModuleNodeModules: [
...acc.haste.providesModuleNodeModules,
...haste.providesModuleNodeModules,
],
platforms: [...acc.haste.platforms, ...haste.platforms],
},
}): ConfigT);
}, initialConfig);

if (depsWithWarnings.length) {
logger.warn(
Expand Down
19 changes: 18 additions & 1 deletion packages/cli/src/tools/config/readConfigFromDisk.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,24 @@ const loadProjectCommands = (
function readLegacyDependencyConfigFromDisk(
rootFolder: string,
): ?UserDependencyConfigT {
const {rnpm: config} = require(path.join(rootFolder, 'package.json'));
let config = {};

try {
config = require(path.join(rootFolder, 'package.json')).rnpm;
} catch (error) {
// package.json is usually missing in local libraries that are not in
// project "dependencies", so we just return a bare config
return {
dependency: {
platforms: {},
assets: [],
hooks: {},
params: [],
},
commands: [],
platforms: {},
};
}

if (!config) {
return undefined;
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/tools/config/schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export const projectConfig = t
t.string(),
t
.object({
root: t.string(),
platforms: map(t.string(), t.any()).keys({
ios: t
.object({
Expand Down
4 changes: 2 additions & 2 deletions types/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,8 @@ export type UserDependencyConfigT = {
// Additional dependency settings
dependency: {
platforms: {
android: DependencyParamsAndroidT,
ios: ProjectParamsIOST,
android?: DependencyParamsAndroidT,
ios?: ProjectParamsIOST,
[key: string]: any,
},
assets: string[],
Expand Down