Skip to content

Commit

Permalink
Fix: Codegen template error in RCTThirdPartyFabricComponentsProvider (#…
Browse files Browse the repository at this point in the history
…34738)

Summary:
When `GenerateRCTThirdPartyFabricComponentsProviderCpp.js` generates `RCTThirdPartyFabricComponentsProvider.mm` an edge case happens in the following situation:
- The same library exports multiple modules with one component each (i.e. one component per file);
- The **first component** is excluded for iOS via the `excludedPlatforms` property in *codegenNativeComponent*.

A "loose" comma appears in the generated template, breaking the code.

```c++
Class<RCTComponentViewProtocol> RCTThirdPartyFabricComponentsProvider(const char *name) {
  static std::unordered_map<std::string, Class (*)(void)> sFabricComponentsClassMap = {
, // <-- the offending comma
    {"NativeComponent2", NativeComponent2Cls}, // rnmylibrary
  };
}
```

At some point, `GenerateRCTThirdPartyFabricComponentsProviderCpp.js` does not properly filter out empty arrays resulting from excluded components. This does not seem to be a problem when the excluded component is not the first being processed, as the comma gets added at the end of the previous line, after the comment with the name of the library.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Fixed] - Fix error in the Codegen template for ThirdPartyFabricComponentsProvider

Pull Request resolved: #34738

Test Plan:
<!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. -->

This is the schema that leads to the bug. Notice that the first component was excluded for iOS.
```json
{
  "modules": {
    "ComponentFile1": {
      "type": "Component",
      "components": {
        "NativeComponent1": {
          "excludedPlatforms": ["iOS"]
          "extendsProps": [
            {
              "type": "ReactNativeBuiltInType",
              "knownTypeName": "ReactNativeCoreViewProps"
            }
          ],
          "events": [],
          "props": [],
          "commands": []
        }
      }
    },
    "ComponentFile2": {
      "type": "Component",
      "components": {
        "NativeComponent2": {
          "extendsProps": [
            {
              "type": "ReactNativeBuiltInType",
              "knownTypeName": "ReactNativeCoreViewProps"
            }
          ],
          "events": [],
          "props": [],
          "commands": []
        }
      }
    }
  }
```

`GenerateRCTThirdPartyFabricComponentsProviderCpp.js` should generate a template without the comma in the wrong position (before NativeComponent2).

I also added an additional test case to cover this problem. All the other tests passed.

Reviewed By: sammy-SC

Differential Revision: D39686573

Pulled By: cipolleschi

fbshipit-source-id: 6054464d024218eb0b2e02974aa5cc7c8aebbbc9
  • Loading branch information
gispada authored and facebook-github-bot committed Sep 21, 2022
1 parent 8745a14 commit 2f6b212
Show file tree
Hide file tree
Showing 17 changed files with 571 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ module.exports = {
return null;
}

return Object.keys(components)
const componentTemplates = Object.keys(components)
.filter(componentName => {
const component = components[componentName];
return !(
Expand All @@ -91,6 +91,8 @@ module.exports = {

return replacedTemplate;
});

return componentTemplates.length > 0 ? componentTemplates : null;
})
.filter(Boolean);
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,53 @@ const EXCLUDE_ANDROID_IOS: SchemaType = {
},
};

const EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES: SchemaType = {
modules: {
ComponentFile1: {
type: 'Component',
components: {
ExcludedIosComponent: {
excludedPlatforms: ['iOS'],
extendsProps: [
{
type: 'ReactNativeBuiltInType',
knownTypeName: 'ReactNativeCoreViewProps',
},
],
events: [],
props: [],
commands: [],
},
},
},
ComponentFile2: {
type: 'Component',
components: {
MultiFileIncludedNativeComponent: {
extendsProps: [
{
type: 'ReactNativeBuiltInType',
knownTypeName: 'ReactNativeCoreViewProps',
},
],
events: [],
props: [
{
name: 'disabled',
optional: true,
typeAnnotation: {
type: 'BooleanTypeAnnotation',
default: true,
},
},
],
commands: [],
},
},
},
},
};

module.exports = {
NO_PROPS_NO_EVENTS,
INTERFACE_ONLY,
Expand Down Expand Up @@ -1621,4 +1668,5 @@ module.exports = {
COMMANDS_AND_PROPS,
EXCLUDE_ANDROID,
EXCLUDE_ANDROID_IOS,
EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES,
};
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,35 @@ using ExcludedAndroidIosComponentComponentDescriptor = ConcreteComponentDescript
}
`;
exports[`GenerateComponentDescriptorH can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"ComponentDescriptors.h" => "
/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GenerateComponentDescriptorH.js
*/
#pragma once
#include <react/renderer/components/EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES/ShadowNodes.h>
#include <react/renderer/core/ConcreteComponentDescriptor.h>
namespace facebook {
namespace react {
using ExcludedIosComponentComponentDescriptor = ConcreteComponentDescriptor<ExcludedIosComponentShadowNode>;
using MultiFileIncludedNativeComponentComponentDescriptor = ConcreteComponentDescriptor<MultiFileIncludedNativeComponentShadowNode>;
} // namespace react
} // namespace facebook
",
}
`;
exports[`GenerateComponentDescriptorH can generate fixture FLOAT_PROPS 1`] = `
Map {
"ComponentDescriptors.h" => "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,31 @@ NS_ASSUME_NONNULL_BEGIN
NS_ASSUME_NONNULL_END",
}
`;
exports[`GenerateComponentHObjCpp can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"RCTComponentViewHelpers.h" => "/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GenerateComponentHObjCpp.js
*/
#import <Foundation/Foundation.h>
#import <React/RCTDefines.h>
#import <React/RCTLog.h>
NS_ASSUME_NONNULL_BEGIN
@protocol RCTMultiFileIncludedNativeComponentViewProtocol <NSObject>
@end
NS_ASSUME_NONNULL_END",
}
`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,32 @@ namespace react {
} // namespace react
} // namespace facebook
",
}
`;
exports[`GenerateEventEmitterCpp can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"EventEmitters.cpp" => "
/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GenerateEventEmitterCpp.js
*/
#include <react/renderer/components/EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES/EventEmitters.h>
namespace facebook {
namespace react {
} // namespace react
} // namespace facebook
",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,33 @@ namespace react {
} // namespace react
} // namespace facebook
",
}
`;
exports[`GenerateEventEmitterH can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"EventEmitters.h" => "
/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GenerateEventEmitterH.js
*/
#pragma once
#include <react/renderer/components/view/ViewEventEmitter.h>
#include <jsi/jsi.h>
namespace facebook {
namespace react {
} // namespace react
} // namespace facebook
",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,46 @@ ExcludedAndroidIosComponentProps::ExcludedAndroidIosComponentProps(
}
`;
exports[`GeneratePropsCpp can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"Props.cpp" => "
/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GeneratePropsCpp.js
*/
#include <react/renderer/components/EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES/Props.h>
#include <react/renderer/core/PropsParserContext.h>
#include <react/renderer/core/propsConversions.h>
namespace facebook {
namespace react {
ExcludedIosComponentProps::ExcludedIosComponentProps(
const PropsParserContext &context,
const ExcludedIosComponentProps &sourceProps,
const RawProps &rawProps): ViewProps(context, sourceProps, rawProps)
{}
MultiFileIncludedNativeComponentProps::MultiFileIncludedNativeComponentProps(
const PropsParserContext &context,
const MultiFileIncludedNativeComponentProps &sourceProps,
const RawProps &rawProps): ViewProps(context, sourceProps, rawProps),
disabled(convertRawProp(context, rawProps, \\"disabled\\", sourceProps.disabled, {true}))
{}
} // namespace react
} // namespace facebook
",
}
`;
exports[`GeneratePropsCpp can generate fixture FLOAT_PROPS 1`] = `
Map {
"Props.cpp" => "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,52 @@ class JSI_EXPORT ExcludedAndroidIosComponentProps final : public ViewProps {
}
`;
exports[`GeneratePropsH can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"Props.h" => "
/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GeneratePropsH.js
*/
#pragma once
#include <jsi/jsi.h>
#include <react/renderer/components/view/ViewProps.h>
#include <react/renderer/core/PropsParserContext.h>
namespace facebook {
namespace react {
class JSI_EXPORT ExcludedIosComponentProps final : public ViewProps {
public:
ExcludedIosComponentProps() = default;
ExcludedIosComponentProps(const PropsParserContext& context, const ExcludedIosComponentProps &sourceProps, const RawProps &rawProps);
#pragma mark - Props
};
class JSI_EXPORT MultiFileIncludedNativeComponentProps final : public ViewProps {
public:
MultiFileIncludedNativeComponentProps() = default;
MultiFileIncludedNativeComponentProps(const PropsParserContext& context, const MultiFileIncludedNativeComponentProps &sourceProps, const RawProps &rawProps);
#pragma mark - Props
bool disabled{true};
};
} // namespace react
} // namespace facebook
",
}
`;
exports[`GeneratePropsH can generate fixture FLOAT_PROPS 1`] = `
Map {
"Props.h" => "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,69 @@ exports[`GeneratePropsJavaDelegate can generate fixture EXCLUDE_ANDROID 1`] = `M
exports[`GeneratePropsJavaDelegate can generate fixture EXCLUDE_ANDROID_IOS 1`] = `Map {}`;
exports[`GeneratePropsJavaDelegate can generate fixture EXCLUDE_IOS_TWO_COMPONENTS_DIFFERENT_FILES 1`] = `
Map {
"java/com/facebook/react/viewmanagers/ExcludedIosComponentManagerDelegate.java" => "/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GeneratePropsJavaDelegate.js
*/
package com.facebook.react.viewmanagers;
import android.view.View;
import androidx.annotation.Nullable;
import com.facebook.react.uimanager.BaseViewManagerDelegate;
import com.facebook.react.uimanager.BaseViewManagerInterface;
public class ExcludedIosComponentManagerDelegate<T extends View, U extends BaseViewManagerInterface<T> & ExcludedIosComponentManagerInterface<T>> extends BaseViewManagerDelegate<T, U> {
public ExcludedIosComponentManagerDelegate(U viewManager) {
super(viewManager);
}
@Override
public void setProperty(T view, String propName, @Nullable Object value) {
super.setProperty(view, propName, value);
}
}
",
"java/com/facebook/react/viewmanagers/MultiFileIncludedNativeComponentManagerDelegate.java" => "/**
* This code was generated by [react-native-codegen](https://www.npmjs.com/package/react-native-codegen).
*
* Do not edit this file as changes may cause incorrect behavior and will be lost
* once the code is regenerated.
*
* @generated by codegen project: GeneratePropsJavaDelegate.js
*/
package com.facebook.react.viewmanagers;
import android.view.View;
import androidx.annotation.Nullable;
import com.facebook.react.uimanager.BaseViewManagerDelegate;
import com.facebook.react.uimanager.BaseViewManagerInterface;
public class MultiFileIncludedNativeComponentManagerDelegate<T extends View, U extends BaseViewManagerInterface<T> & MultiFileIncludedNativeComponentManagerInterface<T>> extends BaseViewManagerDelegate<T, U> {
public MultiFileIncludedNativeComponentManagerDelegate(U viewManager) {
super(viewManager);
}
@Override
public void setProperty(T view, String propName, @Nullable Object value) {
switch (propName) {
case \\"disabled\\":
mViewManager.setDisabled(view, value == null ? true : (boolean) value);
break;
default:
super.setProperty(view, propName, value);
}
}
}
",
}
`;
exports[`GeneratePropsJavaDelegate can generate fixture FLOAT_PROPS 1`] = `
Map {
"java/com/facebook/react/viewmanagers/FloatPropNativeComponentManagerDelegate.java" => "/**
Expand Down
Loading

0 comments on commit 2f6b212

Please sign in to comment.