Skip to content

Commit

Permalink
Yolo Delete ReactNativePropRegistry
Browse files Browse the repository at this point in the history
Summary:
Changed StyleSheet.create to be the identity function. We no longer hide it behind an opaque number. Better for types and perf since we don't use it.

I don't really know if we have/need any safer way of rolling this out than just landing it.

It can break if the object passed to StyleSheet.create is mutated afterwards but that isn't a practice anywhere I've seen.

Reviewed By: sophiebits

Differential Revision: D7530023

fbshipit-source-id: bc1afa879c5a5d9cd95cb13bc8ff3347b3622851
  • Loading branch information
sebmarkbage authored and facebook-github-bot committed Apr 10, 2018
1 parent 722f88c commit a8e3c7f
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 61 deletions.
18 changes: 0 additions & 18 deletions Libraries/Renderer/shims/ReactNativePropRegistry.js

This file was deleted.

32 changes: 16 additions & 16 deletions Libraries/StyleSheet/StyleSheet.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@
'use strict';

const PixelRatio = require('PixelRatio');
const ReactNativePropRegistry = require('ReactNativePropRegistry');
const ReactNativeStyleAttributes = require('ReactNativeStyleAttributes');
const StyleSheetValidation = require('StyleSheetValidation');

const flatten = require('flattenStyle');

import type {
____StyleSheetInternalStyleIdentifier_Internal as StyleSheetInternalStyleIdentifier,
____Styles_Internal,
____DangerouslyImpreciseStyle_Internal,
____DangerouslyImpreciseStyleProp_Internal,
Expand Down Expand Up @@ -171,16 +169,16 @@ if (hairlineWidth === 0) {
hairlineWidth = 1 / PixelRatio.get();
}

const absoluteFillObject: LayoutStyle = {
const absoluteFill: LayoutStyle = {
position: 'absolute',
left: 0,
right: 0,
top: 0,
bottom: 0,
};
const absoluteFill: StyleSheetInternalStyleIdentifier = ReactNativePropRegistry.register(
absoluteFillObject,
); // This also freezes it
if (__DEV__) {
Object.freeze(absoluteFill);
}

/**
* A StyleSheet is an abstraction similar to CSS StyleSheets
Expand Down Expand Up @@ -253,7 +251,7 @@ module.exports = {
* so `absoluteFill` can be used for convenience and to reduce duplication of these repeated
* styles.
*/
absoluteFill,
absoluteFill: (absoluteFill: any), // TODO: This should be updated after we fix downstream Flow sites.

/**
* Sometimes you may want `absoluteFill` but with a couple tweaks - `absoluteFillObject` can be
Expand All @@ -267,7 +265,7 @@ module.exports = {
* },
* });
*/
absoluteFillObject,
absoluteFillObject: absoluteFill,

/**
* Combines two styles such that `style2` will override any styles in `style1`.
Expand Down Expand Up @@ -361,14 +359,16 @@ module.exports = {
/**
* Creates a StyleSheet style reference from the given object.
*/
create<+S: ____Styles_Internal>(
obj: S,
): $ObjMap<S, (Object) => StyleSheetInternalStyleIdentifier> {
const result = {};
for (const key in obj) {
StyleSheetValidation.validateStyle(key, obj);
result[key] = obj[key] && ReactNativePropRegistry.register(obj[key]);
create<+S: ____Styles_Internal>(obj: S): $ObjMap<S, (Object) => any> {
// TODO: This should return S as the return type. But first,
// we need to codemod all the callsites that are typing this
// return value as a number (even though it was opaque).
if (__DEV__) {
for (const key in obj) {
StyleSheetValidation.validateStyle(key, obj);
Object.freeze(obj[key]);

This comment has been minimized.

Copy link
@Bibazavr

Bibazavr May 31, 2022

Why it should be freeze in dev?

}
}
return result;
return obj;

This comment has been minimized.

Copy link
@brunolemos

brunolemos Nov 28, 2018

Contributor

It's not clear the value StyleSheet.create provides now. Should it be deprecated? Or maybe put an explanation in the docs saying why should we ever use it and that it doesn't provide any optimization over creating a simple object?

cc facebook/react-native-website#629 (comment)

This comment has been minimized.

Copy link
@danielmarino24i

danielmarino24i Dec 17, 2018

So what should be the proper usage instead of StyleSheet.create?

},
};
4 changes: 0 additions & 4 deletions Libraries/StyleSheet/StyleSheetTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@

const AnimatedNode = require('AnimatedNode');

export opaque type ____StyleSheetInternalStyleIdentifier_Internal: number = number;

export type ColorValue = null | string;
export type DimensionValue = null | number | string | AnimatedNode;

Expand Down Expand Up @@ -224,8 +222,6 @@ type GenericStyleProp<+T> =
| null
| void
| T
| ____StyleSheetInternalStyleIdentifier_Internal
| number
| false
| ''
| $ReadOnlyArray<GenericStyleProp<T>>;
Expand Down
2 changes: 1 addition & 1 deletion Libraries/StyleSheet/__tests__/flattenStyle-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ describe('flattenStyle', () => {
it('should ignore invalid class names', () => {
var invalid = flattenStyle(1234, null);

expect(invalid).toEqual({});
expect(invalid).toEqual(undefined);
// Invalid class name 1234 skipping ...
});
});
19 changes: 2 additions & 17 deletions Libraries/StyleSheet/flattenStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,20 @@
*/
'use strict';

var ReactNativePropRegistry;

import type {
DangerouslyImpreciseStyle,
DangerouslyImpreciseStyleProp,
} from 'StyleSheet';

function getStyle(style) {
if (ReactNativePropRegistry === undefined) {
ReactNativePropRegistry = require('ReactNativePropRegistry');
}
if (typeof style === 'number') {
return ReactNativePropRegistry.getByID(style);
}
return style;
}

function flattenStyle(
style: ?DangerouslyImpreciseStyleProp,
): ?DangerouslyImpreciseStyle {
if (style == null) {
if (style === null || typeof style !== 'object') {
return undefined;
}

if (!Array.isArray(style)) {
/* $FlowFixMe(>=0.63.0 site=react_native_fb) This comment suppresses an
* error found when Flow v0.63 was deployed. To see the error delete this
* comment and run Flow. */
return getStyle(style);
return style;
}

var result = {};
Expand Down
6 changes: 1 addition & 5 deletions jest/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,11 +324,7 @@ Object.keys(mockNativeModules).forEach(module => {
});

jest
.doMock('NativeModules', () => mockNativeModules)
.doMock('ReactNativePropRegistry', () => ({
register: id => id,
getByID: () => mockEmptyObject,
}));
.doMock('NativeModules', () => mockNativeModules);

jest.doMock('requireNativeComponent', () => {
const React = require('react');
Expand Down

2 comments on commit a8e3c7f

@slorber
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @sebmarkbage

If there any discussion related to why you remove that registry and remove the supposed performance benefits that StyleSheet.create is supposed to bring over plain inline styles? Can't find this in the repo

Context: https://twitter.com/sebastienlorber/status/1067688788611264512

@slorber
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the ReactNativePropRegistry never sent the registered styles to the bridge but rather kept them in memory in js side.

Please sign in to comment.