From 635124f175aad21975dc9f5f63c890357ebb1a16 Mon Sep 17 00:00:00 2001 From: Riccardo Cipolleschi Date: Thu, 28 Apr 2022 03:59:40 -0700 Subject: [PATCH] Update CodeGen to leverage the `outputDir` as suggested in diff review (#33729) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/33729 This PR addresses [this comment](https://www.internalfb.com/diff/D35820848?dst_version_fbid=496290878846487&transaction_fbid=355967423221044). It makes the CodeGen to the `outputDir` as base directory for the codegen. Finally, it updates the unit tests accordingly. ## Changelog [iOS][Changed] - use `outputDir` as base directory for the codegen and remove the possibility to customize the intermediate path. The generated code requires specific paths in the `#include` directive. Reviewed By: cortinico, dmitryrykun Differential Revision: D35935282 fbshipit-source-id: 65806090bf31384ce8d4b12d192041afb1b726b5 --- packages/react-native-codegen/DEFS.bzl | 4 +- .../src/generators/RNCodegen.js | 15 ++++--- .../generators/__tests__/RNCodegen-test.js | 21 +++++---- .../generate-artifacts-executor-test.js | 42 +++-------------- .../generate-specs-cli-executor-test.js | 22 ++++----- .../codegen/generate-artifacts-executor.js | 45 +++---------------- .../codegen/generate-specs-cli-executor.js | 22 ++++----- scripts/generate-specs-cli.js | 12 +---- 8 files changed, 60 insertions(+), 123 deletions(-) diff --git a/packages/react-native-codegen/DEFS.bzl b/packages/react-native-codegen/DEFS.bzl index aafc1d2a7a8679..6f36f86d87a4c3 100644 --- a/packages/react-native-codegen/DEFS.bzl +++ b/packages/react-native-codegen/DEFS.bzl @@ -220,14 +220,14 @@ def rn_codegen_modules( # iOS Buck build isn't fully working in OSS, so let's skip it for OSS for now. fb_native.genrule( name = generate_module_hobjcpp_name, - cmd = "cp $(location :{})/{}.h $OUT".format(generate_fixtures_rule_name, name), + cmd = "cp $(location :{})/{}/{}.h $OUT".format(generate_fixtures_rule_name, name, name), out = "{}.h".format(name), labels = ["codegen_rule"], ) fb_native.genrule( name = generate_module_mm_name, - cmd = "cp $(location :{})/{}-generated.mm $OUT".format(generate_fixtures_rule_name, name), + cmd = "cp $(location :{})/{}/{}-generated.mm $OUT".format(generate_fixtures_rule_name, name, name), out = "{}-generated.mm".format(name), labels = ["codegen_rule"], ) diff --git a/packages/react-native-codegen/src/generators/RNCodegen.js b/packages/react-native-codegen/src/generators/RNCodegen.js index d9487e8832fe5f..dd9cd1283958cc 100644 --- a/packages/react-native-codegen/src/generators/RNCodegen.js +++ b/packages/react-native-codegen/src/generators/RNCodegen.js @@ -48,8 +48,6 @@ type LibraryOptions = $ReadOnly<{ outputDirectory: string, packageName?: string, // Some platforms have a notion of package, which should be configurable. assumeNonnull: boolean, - componentsOutputDir?: string, // optional for backward compatibility - modulesOutputDir?: string, // optional for backward compatibility }>; type SchemasOptions = $ReadOnly<{ @@ -195,16 +193,21 @@ module.exports = { outputDirectory, packageName, assumeNonnull, - componentsOutputDir, - modulesOutputDir, }: LibraryOptions, {generators, test}: LibraryConfig, ): boolean { schemaValidator.validate(schema); + function composePath(intermediate) { + return path.join(outputDirectory, intermediate, libraryName); + } + + const componentIOSOutput = composePath('react/renderer/components/'); + const modulesIOSOutput = composePath('./'); + const outputFoldersForGenerators = { - componentsIOS: componentsOutputDir ?? outputDirectory, // fallback for backward compatibility - modulesIOS: modulesOutputDir ?? outputDirectory, // fallback for backward compatibility + componentsIOS: componentIOSOutput, + modulesIOS: modulesIOSOutput, descriptors: outputDirectory, events: outputDirectory, props: outputDirectory, diff --git a/packages/react-native-codegen/src/generators/__tests__/RNCodegen-test.js b/packages/react-native-codegen/src/generators/__tests__/RNCodegen-test.js index a529379d72eecf..4cc51277d1d03d 100644 --- a/packages/react-native-codegen/src/generators/__tests__/RNCodegen-test.js +++ b/packages/react-native-codegen/src/generators/__tests__/RNCodegen-test.js @@ -15,13 +15,17 @@ const rnCodegen = require('../RNCodegen.js'); const fixture = require('../__test_fixtures__/fixtures.js'); const path = require('path'); -const invalidDirectory = 'invalid/'; +const outputDirectory = 'tmp/out/'; const packageName = 'na'; -const componentsOutputDir = 'react/renderer/components/library'; -const modulesOutputDir = 'library'; describe('RNCodegen.generate', () => { - it('when type `all`', () => { + beforeEach(() => { + jest.resetModules(); + }); + it('when type `all`, with default paths', () => { + const componentsOutputDir = 'react/renderer/components/library'; + const modulesOutputDir = 'library'; + const expectedPaths = { 'library.h': modulesOutputDir, 'library-generated.mm': modulesOutputDir, @@ -43,7 +47,10 @@ describe('RNCodegen.generate', () => { let receivedDir = path.dirname(location); let receivedBasename = path.basename(location); - let expectedPath = expectedPaths[receivedBasename]; + let expectedPath = path.join( + outputDirectory, + expectedPaths[receivedBasename], + ); expect(receivedDir).toEqual(expectedPath); }, })); @@ -52,11 +59,9 @@ describe('RNCodegen.generate', () => { { libraryName: 'library', schema: fixture.all, - outputDirectory: invalidDirectory, + outputDirectory: outputDirectory, packageName: packageName, assumeNonnull: true, - componentsOutputDir: componentsOutputDir, - modulesOutputDir: modulesOutputDir, }, { generators: ['componentsIOS', 'modulesIOS'], diff --git a/scripts/codegen/__tests__/generate-artifacts-executor-test.js b/scripts/codegen/__tests__/generate-artifacts-executor-test.js index 6b7addd60288dd..ed106ec820ef09 100644 --- a/scripts/codegen/__tests__/generate-artifacts-executor-test.js +++ b/scripts/codegen/__tests__/generate-artifacts-executor-test.js @@ -24,38 +24,19 @@ describe('generateCode', () => { const rnRoot = path.join(__dirname, '../..'); const libraryType = 'all'; - const tmpComponentOutDirs = path.join(tmpDir, 'out', 'components'); - const tmpNativeModulesOutDir = path.join(tmpDir, 'out', 'nativeModules'); - const iOSComponentOutDirs = path.join( - iosOutputDir, - 'react/renderer/components', - library.config.name, - ); - const iOSNativeModulesOutDir = path.join( - iosOutputDir, - './', - library.config.name, - ); + const tmpOutDir = path.join(tmpDir, 'out'); // mock used functions let mkdirSyncInvocationCount = 0; jest.mock('fs', () => ({ - readdirSync: dirpath => { - return ['test/dir']; //we only require to return something, so that the folder is not empty. - }, mkdirSync: (location, config) => { if (mkdirSyncInvocationCount === 0) { - expect(location).toEqual(tmpComponentOutDirs); + expect(location).toEqual(tmpOutDir); } if (mkdirSyncInvocationCount === 1) { - expect(location).toEqual(tmpNativeModulesOutDir); - } - if (mkdirSyncInvocationCount === 2) { - expect(location).toEqual(iOSComponentOutDirs); - } - if (mkdirSyncInvocationCount === 3) { - expect(location).toEqual(iOSNativeModulesOutDir); + expect(location).toEqual(iosOutputDir); } + mkdirSyncInvocationCount += 1; }, })); @@ -70,23 +51,14 @@ describe('generateCode', () => { )} \ --platform ios \ --schemaPath ${pathToSchema} \ - --outputDir ${tmpNativeModulesOutDir} \ - --componentsOutputDir ${tmpComponentOutDirs} \ - --modulesOutputDirs ${tmpNativeModulesOutDir} \ + --outputDir ${tmpOutDir} \ --libraryName ${library.config.name} \ --libraryType ${libraryType}`; expect(command).toEqual(expectedCommand); } if (execSyncInvocationCount === 1) { - expect(command).toEqual( - `cp -R ${tmpComponentOutDirs}/* ${iOSComponentOutDirs}`, - ); - } - if (execSyncInvocationCount === 2) { - expect(command).toEqual( - `cp -R ${tmpNativeModulesOutDir}/* ${iOSNativeModulesOutDir}`, - ); + expect(command).toEqual(`cp -R ${tmpOutDir}/* ${iosOutputDir}`); } execSyncInvocationCount += 1; @@ -94,6 +66,6 @@ describe('generateCode', () => { })); underTest._generateCode(iosOutputDir, library, tmpDir, node, pathToSchema); - expect(mkdirSyncInvocationCount).toBe(4); + expect(mkdirSyncInvocationCount).toBe(2); }); }); diff --git a/scripts/codegen/__tests__/generate-specs-cli-executor-test.js b/scripts/codegen/__tests__/generate-specs-cli-executor-test.js index 41b8d3f2f28583..ce63546a566bfd 100644 --- a/scripts/codegen/__tests__/generate-specs-cli-executor-test.js +++ b/scripts/codegen/__tests__/generate-specs-cli-executor-test.js @@ -12,16 +12,20 @@ const sut = require('../generate-specs-cli-executor'); const fixtures = require('../__test_fixtures__/fixtures'); +const path = require('path'); describe('generateSpec', () => { it('invokes RNCodegen with the right params', () => { const platform = 'ios'; const libraryType = 'all'; const schemaPath = './'; - const componentsOutputDir = - 'app/ios/build/generated/ios/react/renderer/components/library'; - const modulesOutputDir = 'app/ios/build/generated/ios/./library'; - const outputDirectory = 'app/ios/build/generated/ios'; + const componentsOutputDir = path.normalize( + 'app/ios/build/generated/ios/react/renderer/components/library', + ); + const modulesOutputDir = path.normalize( + 'app/ios/build/generated/ios/library', + ); + const outputDirectory = path.normalize('app/ios/build/generated/ios'); const libraryName = 'library'; const packageName = 'com.library'; const generators = ['componentsIOS', 'modulesIOS']; @@ -38,15 +42,15 @@ describe('generateSpec', () => { jest.mock('mkdirp', () => ({ sync: folder => { if (mkdirpSyncInvoked === 0) { - expect(folder).toBe(componentsOutputDir); + expect(folder).toBe(outputDirectory); } if (mkdirpSyncInvoked === 1) { - expect(folder).toBe(modulesOutputDir); + expect(folder).toBe(componentsOutputDir); } if (mkdirpSyncInvoked === 2) { - expect(folder).toBe(outputDirectory); + expect(folder).toBe(modulesOutputDir); } mkdirpSyncInvoked += 1; @@ -65,8 +69,6 @@ describe('generateSpec', () => { expect(libraryConfig.schema).toStrictEqual(fixtures.schema); expect(libraryConfig.outputDirectory).toBe(outputDirectory); expect(libraryConfig.packageName).toBe(packageName); - expect(libraryConfig.componentsOutputDir).toBe(componentsOutputDir); - expect(libraryConfig.modulesOutputDir).toBe(modulesOutputDir); expect(generatorConfigs.generators).toStrictEqual(generators); expect(generatorConfigs.test).toBeUndefined(); @@ -81,8 +83,6 @@ describe('generateSpec', () => { libraryName, packageName, libraryType, - componentsOutputDir, - modulesOutputDir, ); expect(mkdirpSyncInvoked).toBe(3); diff --git a/scripts/codegen/generate-artifacts-executor.js b/scripts/codegen/generate-artifacts-executor.js index 281ed411f3844b..94278756326ea1 100644 --- a/scripts/codegen/generate-artifacts-executor.js +++ b/scripts/codegen/generate-artifacts-executor.js @@ -38,10 +38,6 @@ function executeNodeScript(node, script) { execSync(`${node} ${script}`); } -function isDirEmpty(dirPath) { - return fs.readdirSync(dirPath).length === 0; -} - function isAppRootValid(appRootDir) { if (appRootDir == null) { console.error('Missing path to React Native application'); @@ -212,57 +208,28 @@ function generateSchema(tmpDir, library, node, codegenCliPath) { } function generateCode(iosOutputDir, library, tmpDir, node, pathToSchema) { - function composePath(intermediate) { - return path.join(iosOutputDir, intermediate, library.config.name); - } - - const outputDirsIOS = { - components: composePath('react/renderer/components'), - nativeModules: composePath('./'), - }; - - const tempOutputDirs = { - components: path.join(tmpDir, 'out', 'components'), - nativeModules: path.join(tmpDir, 'out', 'nativeModules'), - }; - // ...then generate native code artifacts. const libraryTypeArg = library.config.type ? `--libraryType ${library.config.type}` : ''; - Object.entries(tempOutputDirs).forEach(([type, dirPath]) => { - fs.mkdirSync(dirPath, {recursive: true}); - }); - - const deprecated_outputDir = - library.config.type === 'components' - ? tempOutputDirs.components - : tempOutputDirs.nativeModules; + const tmpOutputDir = path.join(tmpDir, 'out'); + fs.mkdirSync(tmpOutputDir, {recursive: true}); executeNodeScript( node, `${path.join(RN_ROOT, 'scripts', 'generate-specs-cli.js')} \ --platform ios \ --schemaPath ${pathToSchema} \ - --outputDir ${deprecated_outputDir} \ - --componentsOutputDir ${tempOutputDirs.components} \ - --modulesOutputDirs ${tempOutputDirs.nativeModules} \ + --outputDir ${tmpOutputDir} \ --libraryName ${library.config.name} \ ${libraryTypeArg}`, ); // Finally, copy artifacts to the final output directory. - Object.entries(outputDirsIOS).forEach(([type, dirPath]) => { - const outDir = tempOutputDirs[type]; - if (isDirEmpty(outDir)) { - return; // cp fails if we try to copy something empty. - } - - fs.mkdirSync(dirPath, {recursive: true}); - execSync(`cp -R ${outDir}/* ${dirPath}`); - console.log(`[Codegen] Generated artifacts: ${dirPath}`); - }); + fs.mkdirSync(iosOutputDir, {recursive: true}); + execSync(`cp -R ${tmpOutputDir}/* ${iosOutputDir}`); + console.log(`[Codegen] Generated artifacts: ${iosOutputDir}`); } function generateNativeCodegenFiles( diff --git a/scripts/codegen/generate-specs-cli-executor.js b/scripts/codegen/generate-specs-cli-executor.js index 7ce95807b91134..fe37b07fd9db88 100644 --- a/scripts/codegen/generate-specs-cli-executor.js +++ b/scripts/codegen/generate-specs-cli-executor.js @@ -30,10 +30,7 @@ const GENERATORS = { }, }; -function deprecated_createOutputDirectoryIfNeeded( - outputDirectory, - libraryName, -) { +function createOutputDirectoryIfNeeded(outputDirectory, libraryName) { if (!outputDirectory) { outputDirectory = path.resolve(__dirname, '..', 'Libraries', libraryName); } @@ -81,16 +78,21 @@ function generateSpec( libraryName, packageName, libraryType, - componentsOutputDir, - modulesOutputDir, ) { validateLibraryType(libraryType); let schema = readAndParseSchema(schemaPath); - createFolderIfDefined(componentsOutputDir); - createFolderIfDefined(modulesOutputDir); - deprecated_createOutputDirectoryIfNeeded(outputDirectory, libraryName); + createOutputDirectoryIfNeeded(outputDirectory, libraryName); + function composePath(intermediate) { + return path.join(outputDirectory, intermediate, libraryName); + } + + // These are hardcoded and should not be changed. + // The codegen creates some C++ code with #include directive + // which uses these paths. Those directive are not customizable yet. + createFolderIfDefined(composePath('react/renderer/components/')); + createFolderIfDefined(composePath('./')); RNCodegen.generate( { @@ -98,8 +100,6 @@ function generateSpec( schema, outputDirectory, packageName, - componentsOutputDir, - modulesOutputDir, }, { generators: GENERATORS[libraryType][platform], diff --git a/scripts/generate-specs-cli.js b/scripts/generate-specs-cli.js index 3e5bc9549e3820..19862d20131d97 100644 --- a/scripts/generate-specs-cli.js +++ b/scripts/generate-specs-cli.js @@ -24,7 +24,7 @@ const argv = yargs .option('o', { alias: 'outputDir', describe: - 'DEPRECATED - Path to directory where native code source files should be saved.', + 'Path to the root directory where native code source files should be saved.', }) .option('n', { alias: 'libraryName', @@ -41,14 +41,6 @@ const argv = yargs describe: 'all, components, or modules.', default: 'all', }) - .option('c', { - alias: 'componentsOutputDir', - describe: 'Output directory for the codeGen for Fabric Components', - }) - .option('m', { - alias: 'modulesOutputDirs', - describe: 'Output directory for the codeGen for TurboModules', - }) .usage('Usage: $0 ') .demandOption( ['platform', 'schemaPath', 'outputDir'], @@ -63,8 +55,6 @@ function main() { argv.libraryName, argv.javaPackageName, argv.libraryType, - argv.componentsOutputDir, - argv.modulesOutputDirs, ); }