Update CodeGen to leverage the outputDir as suggested in diff review (

#33729) Summary: Pull Request resolved: #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: a9ad4e296efb042cf34b20db5eebb59614beb5f6
facebook · May 4, 2022 · e4d0153 · e4d0153
1 parent 0465c3f
commit e4d0153
Show file tree

Hide file tree

Showing 11 changed files with 68 additions and 128 deletions.
diff --git a/package.json b/package.json
@@ -42,6 +42,9 @@
     "scripts/generate-artifacts.js",
     "scripts/generate-provider-cli.js",
     "scripts/generate-specs-cli.js",
+    "scripts/codegen/codegen-utils.js",
+    "scripts/codegen/generate-artifacts-executor.js",
+    "scripts/codegen/generate-specs-cli-executor.js",
     "scripts/ios-configure-glog.sh",
     "scripts/xcode/with-environment.sh",
     "scripts/launchPackager.bat",

diff --git 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
@@ -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
@@ -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
@@ -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,30 +51,21 @@ 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;
       },
     }));
 
     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
@@ -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/codegen-utils.js b/scripts/codegen/codegen-utils.js
@@ -24,7 +24,7 @@ function getCodegen() {
   try {
     RNCodegen = require('../../packages/react-native-codegen/lib/generators/RNCodegen.js');
   } catch (e) {
-    RNCodegen = require('../react-native-codegen/lib/generators/RNCodegen.js');
+    RNCodegen = require('react-native-codegen/lib/generators/RNCodegen.js');
   }
   if (!RNCodegen) {
     throw 'RNCodegen not found.';

diff --git 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
@@ -30,10 +30,7 @@ const GENERATORS = {
   },
 };
 
-function deprecated_createOutputDirectoryIfNeeded(
-  outputDirectory,
-  libraryName,
-) {
+function createOutputDirectoryIfNeeded(outputDirectory, libraryName) {
   if (!outputDirectory) {
     outputDirectory = path.resolve(__dirname, '..', 'Libraries', libraryName);
   }
@@ -81,25 +78,28 @@ 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(
     {
       libraryName,
       schema,
       outputDirectory,
       packageName,
-      componentsOutputDir,
-      modulesOutputDir,
     },
     {
       generators: GENERATORS[libraryType][platform],