Skip to content

refactor: Convert Animated directory to use ESModule imports/exports #34539

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

Closed
gabrieldonadel wants to merge 3 commits into from
Closed

refactor: Convert Animated directory to use ESModule imports/exports #34539

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
67c84c6
refactor: Convert Animated directory to use ESModule imports/exports
gabrieldonadel Aug 31, 2022
c300338
chore: Update NativeAnimatedHelper imports
gabrieldonadel Sep 1, 2022
f45fb04
chore: Update AnimatedProps imports
gabrieldonadel Sep 1, 2022
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: 9 additions & 9 deletions Libraries/Animated/Animated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,31 @@ import typeof AnimatedSectionList from './components/AnimatedSectionList';
import typeof AnimatedText from './components/AnimatedText';
import typeof AnimatedView from './components/AnimatedView';

import * as AnimatedMock from './AnimatedMock';
import * as AnimatedImplementation from './AnimatedImplementation';
import AnimatedMock from './AnimatedMock';
import AnimatedImplementation from './AnimatedImplementation';

const Animated = ((Platform.isTesting
? AnimatedMock
: AnimatedImplementation): typeof AnimatedMock);

module.exports = {
export default {
get FlatList(): AnimatedFlatList {
return require('./components/AnimatedFlatList');
return require('./components/AnimatedFlatList').default;
},
get Image(): AnimatedImage {
return require('./components/AnimatedImage');
return require('./components/AnimatedImage').default;
},
get ScrollView(): AnimatedScrollView {
return require('./components/AnimatedScrollView');
return require('./components/AnimatedScrollView').default;
},
get SectionList(): AnimatedSectionList {
return require('./components/AnimatedSectionList');
return require('./components/AnimatedSectionList').default;
},
get Text(): AnimatedText {
return require('./components/AnimatedText');
return require('./components/AnimatedText').default;
},
get View(): AnimatedView {
return require('./components/AnimatedView');
return require('./components/AnimatedView').default;
},
...Animated,
};
Copy link
Member

@javache javache Aug 31, 2022

Choose a reason for hiding this comment

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

I think we'll want to keep this, as it enables lazy initialization of these modules (if you don't have the inline-requires transform enabled)

Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

Got it, I'll revert this

Copy link
Contributor

@TMaszko TMaszko Aug 31, 2022

Choose a reason for hiding this comment

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

@javache What do you mean by this if you don't have the inline-requires transform enabled ? Could you explain it a little bit more ?

Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

Reverted

Copy link
Member

@javache javache Sep 1, 2022

Choose a reason for hiding this comment

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

@TMaszko see https://reactnative.dev/docs/ram-bundles-inline-requires for details on inline-requires. It's an important transform we apply to improve bundle init time.

Copy link
Contributor

@TMaszko TMaszko Sep 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@javache I know that optimization technique. My question is why getters do enable lazy initialization when inline-requires transform is disabled? I thought it will work only when this transform is enabled that's why I wanted you to explain it ;)

Copy link
Member

@javache javache Sep 2, 2022

Choose a reason for hiding this comment

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

Ah, sorry for misunderstanding. In this case the require is manually inlined, and we would only invoke the module factory for that module once the getter is invoked. When using import syntax, that manual inlining is not possible.

When the inline-requires transform is enabled, you get this behaviour for free (as long as you're using a getter in this case)

20 changes: 8 additions & 12 deletions Libraries/Animated/AnimatedEvent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@

'use strict';

const AnimatedValue = require('./nodes/AnimatedValue');
const AnimatedValueXY = require('./nodes/AnimatedValueXY');
const NativeAnimatedHelper = require('./NativeAnimatedHelper');
const {findNodeHandle} = require('../ReactNative/RendererProxy');
import AnimatedValue from './nodes/AnimatedValue';
import AnimatedValueXY from './nodes/AnimatedValueXY';
import NativeAnimatedHelper from './NativeAnimatedHelper';
import {findNodeHandle} from '../ReactNative/RendererProxy';

const invariant = require('invariant');

const {shouldUseNativeDriver} = require('./NativeAnimatedHelper');
import invariant from 'invariant';

import type {PlatformConfig} from './AnimatedPlatformConfig';

Expand All @@ -31,7 +29,7 @@ export type EventConfig = {
platformConfig?: PlatformConfig,
};

function attachNativeEvent(
export function attachNativeEvent(
viewRef: any,
eventName: string,
argMapping: $ReadOnlyArray<?Mapping>,
Expand Down Expand Up @@ -146,7 +144,7 @@ function validateMapping(argMapping: $ReadOnlyArray<?Mapping>, args: any) {
});
}

class AnimatedEvent {
export class AnimatedEvent {
_argMapping: $ReadOnlyArray<?Mapping>;
_listeners: Array<Function> = [];
_attachedEvent: ?{detach: () => void, ...};
Expand All @@ -165,7 +163,7 @@ class AnimatedEvent {
this.__addListener(config.listener);
}
this._attachedEvent = null;
this.__isNative = shouldUseNativeDriver(config);
this.__isNative = NativeAnimatedHelper.shouldUseNativeDriver(config);
this.__platformConfig = config.platformConfig;
}

Expand Down Expand Up @@ -257,5 +255,3 @@ class AnimatedEvent {
this._listeners.forEach(listener => listener(...args));
};
}

module.exports = {AnimatedEvent, attachNativeEvent};
36 changes: 18 additions & 18 deletions Libraries/Animated/AnimatedImplementation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,23 @@

'use strict';

const {AnimatedEvent, attachNativeEvent} = require('./AnimatedEvent');
const AnimatedAddition = require('./nodes/AnimatedAddition');
const AnimatedDiffClamp = require('./nodes/AnimatedDiffClamp');
const AnimatedDivision = require('./nodes/AnimatedDivision');
const AnimatedInterpolation = require('./nodes/AnimatedInterpolation');
const AnimatedModulo = require('./nodes/AnimatedModulo');
const AnimatedMultiplication = require('./nodes/AnimatedMultiplication');
const AnimatedNode = require('./nodes/AnimatedNode');
const AnimatedSubtraction = require('./nodes/AnimatedSubtraction');
const AnimatedTracking = require('./nodes/AnimatedTracking');
const AnimatedValue = require('./nodes/AnimatedValue');
const AnimatedValueXY = require('./nodes/AnimatedValueXY');
const DecayAnimation = require('./animations/DecayAnimation');
const SpringAnimation = require('./animations/SpringAnimation');
const TimingAnimation = require('./animations/TimingAnimation');

const createAnimatedComponent = require('./createAnimatedComponent');
import {AnimatedEvent, attachNativeEvent} from './AnimatedEvent';
import AnimatedAddition from './nodes/AnimatedAddition';
import AnimatedDiffClamp from './nodes/AnimatedDiffClamp';
import AnimatedDivision from './nodes/AnimatedDivision';
import AnimatedInterpolation from './nodes/AnimatedInterpolation';
import AnimatedModulo from './nodes/AnimatedModulo';
import AnimatedMultiplication from './nodes/AnimatedMultiplication';
import AnimatedNode from './nodes/AnimatedNode';
import AnimatedSubtraction from './nodes/AnimatedSubtraction';
import AnimatedTracking from './nodes/AnimatedTracking';
import AnimatedValue from './nodes/AnimatedValue';
import AnimatedValueXY from './nodes/AnimatedValueXY';
import DecayAnimation from './animations/DecayAnimation';
import SpringAnimation from './animations/SpringAnimation';
import TimingAnimation from './animations/TimingAnimation';

import createAnimatedComponent from './createAnimatedComponent';

import type {
AnimationConfig,
Expand Down Expand Up @@ -575,7 +575,7 @@ export type {AnimatedNumeric as Numeric};
*
* See https://reactnative.dev/docs/animated
*/
module.exports = {
export default {
/**
* Standard value class for driving animations. Typically initialized with
* `new Animated.Value(0);`
Expand Down
16 changes: 8 additions & 8 deletions Libraries/Animated/AnimatedMock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@

import type {EndResult} from './animations/Animation';

const {AnimatedEvent, attachNativeEvent} = require('./AnimatedEvent');
const AnimatedImplementation = require('./AnimatedImplementation');
const AnimatedInterpolation = require('./nodes/AnimatedInterpolation');
const AnimatedNode = require('./nodes/AnimatedNode');
const AnimatedValue = require('./nodes/AnimatedValue');
const AnimatedValueXY = require('./nodes/AnimatedValueXY');
import {AnimatedEvent, attachNativeEvent} from './AnimatedEvent';
import AnimatedImplementation from './AnimatedImplementation';
import AnimatedInterpolation from './nodes/AnimatedInterpolation';
import AnimatedNode from './nodes/AnimatedNode';
import AnimatedValue from './nodes/AnimatedValue';
import AnimatedValueXY from './nodes/AnimatedValueXY';

const createAnimatedComponent = require('./createAnimatedComponent');
import createAnimatedComponent from './createAnimatedComponent';

import type {EndCallback} from './animations/Animation';
import type {TimingAnimationConfig} from './animations/TimingAnimation';
Expand Down Expand Up @@ -168,7 +168,7 @@ const loop = function (

export type {AnimatedNumeric as Numeric};

module.exports = {
export default {
Value: AnimatedValue,
ValueXY: AnimatedValueXY,
Color: AnimatedColor,
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/AnimatedWeb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@

'use strict';

const AnimatedImplementation = require('./AnimatedImplementation');
import AnimatedImplementation from './AnimatedImplementation';

module.exports = {
export default {
...AnimatedImplementation,
/* $FlowFixMe[incompatible-call] createAnimatedComponent expects to receive
* types. Plain intrinsic components can't be typed like this */
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/Easing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ const Easing = {
x2: number,
y2: number,
): (t: number) => number {
const _bezier = require('./bezier');
const _bezier = require('./bezier').default;
return _bezier(x1, y1, x2, y2);
},

Expand Down Expand Up @@ -247,4 +247,4 @@ const Easing = {
},
};

module.exports = Easing;
export default Easing;
2 changes: 1 addition & 1 deletion Libraries/Animated/NativeAnimatedHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ function transformDataType(value: number | string): number | string {
}
}

module.exports = {
export default {
API,
isSupportedColorStyleProp,
isSupportedStyleProp,
Expand Down
9 changes: 2 additions & 7 deletions Libraries/Animated/SpringConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ function dampingFromOrigamiValue(oValue: number) {
return (oValue - 8) * 3 + 25;
}

function fromOrigamiTensionAndFriction(
export function fromOrigamiTensionAndFriction(
tension: number,
friction: number,
): SpringConfigType {
Expand All @@ -34,7 +34,7 @@ function fromOrigamiTensionAndFriction(
};
}

function fromBouncinessAndSpeed(
export function fromBouncinessAndSpeed(
bounciness: number,
speed: number,
): SpringConfigType {
Expand Down Expand Up @@ -96,8 +96,3 @@ function fromBouncinessAndSpeed(
damping: dampingFromOrigamiValue(bouncyFriction),
};
}

module.exports = {
fromOrigamiTensionAndFriction,
fromBouncinessAndSpeed,
};
6 changes: 3 additions & 3 deletions Libraries/Animated/__tests__/Animated-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* @emails oncall+react_native
*/

import AnimatedProps from '../nodes/AnimatedProps';
import TestRenderer from 'react-test-renderer';
import * as React from 'react';

Expand All @@ -21,7 +20,8 @@ jest.mock('../../BatchedBridge/NativeModules', () => ({
},
}));

let Animated = require('../Animated');
let AnimatedProps = require('../nodes/AnimatedProps').default;
let Animated = require('../Animated').default;
Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

Kept it like this because using ES module imports here breaks jest mocks


describe('Animated tests', () => {
beforeEach(() => {
Expand Down Expand Up @@ -692,7 +692,7 @@ describe('Animated tests', () => {

beforeEach(() => {
jest.mock('../../Interaction/InteractionManager');
Animated = require('../Animated');
Animated = require('../Animated').default;
InteractionManager = require('../../Interaction/InteractionManager');
});

Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/__tests__/AnimatedMock-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

'use strict';

const AnimatedMock = require('../AnimatedMock');
const AnimatedImplementation = require('../AnimatedImplementation');
import AnimatedMock from '../AnimatedMock';
import AnimatedImplementation from '../AnimatedImplementation';

describe('Animated Mock', () => {
it('matches implementation keys', () => {
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/__tests__/AnimatedNative-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ jest
import TestRenderer from 'react-test-renderer';
import * as React from 'react';

const Animated = require('../Animated');
const NativeAnimatedHelper = require('../NativeAnimatedHelper');
const Animated = require('../Animated').default;
const NativeAnimatedHelper = require('../NativeAnimatedHelper').default;
Comment on lines +29 to +30
Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

Kept it like this because using ES module imports here breaks jest mocks

Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

@javache please let me know if this approach is ok for test files of if we should do something different

Copy link
Member

@javache javache Sep 1, 2022

Choose a reason for hiding this comment

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

It should be possible to "fix" jest mocks to work with ES module imports too, but I'm no expert.

Copy link
Contributor

@TMaszko TMaszko Sep 1, 2022

Choose a reason for hiding this comment

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

If you have to dynamically import module and reset it for some reason after each test you have to stick to the require unfortunately :(. In this case though ES module imports should work just fine.

Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Sep 1, 2022

Choose a reason for hiding this comment

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

I believe we won't be able to replace any of these requires from jest test files, on these tests we rely on jest.resetModules and that seems to only work with require() (jestjs/jest#3236).
Any thoughts on what we should do? @javache @TMaszko @necolas

Copy link
Contributor

@necolas necolas Sep 1, 2022

Choose a reason for hiding this comment

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

Tests don't matter so much. It's the Animated module syntax that I have to edit every time I sync code to React Native for Web.

Copy link
Contributor

@TMaszko TMaszko Sep 1, 2022

Choose a reason for hiding this comment

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

@gabrieldonadel Sorry, but I haven't spotted jest.resetModules calls. In this case in my opinion we have to just deal with require.


describe('Native Animated', () => {
const NativeAnimatedModule = require('../NativeAnimatedModule').default;
Expand Down
2 changes: 1 addition & 1 deletion Libraries/Animated/__tests__/Easing-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

'use strict';

const Easing = require('../Easing');
import Easing from '../Easing';
describe('Easing', () => {
it('should work with linear', () => {
const easing = Easing.linear;
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/__tests__/Interpolation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@

'use strict';

const AnimatedInterpolation = require('../nodes/AnimatedInterpolation');
const Easing = require('../Easing');
import AnimatedInterpolation from '../nodes/AnimatedInterpolation';
import Easing from '../Easing';

describe('Interpolation', () => {
it('should work with defaults', () => {
Expand Down
2 changes: 1 addition & 1 deletion Libraries/Animated/__tests__/TimingAnimation-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

'use strict';

const TimingAnimation = require('../animations/TimingAnimation');
import TimingAnimation from '../animations/TimingAnimation';

describe('Timing Animation', () => {
it('should return array of 61 items', () => {
Expand Down
2 changes: 1 addition & 1 deletion Libraries/Animated/__tests__/bezier-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

'use strict';

const bezier = require('../bezier');
import bezier from '../bezier';

const identity = function (x: number) {
return x;
Expand Down
4 changes: 2 additions & 2 deletions Libraries/Animated/__tests__/createAnimatedComponentInjection-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@

'use strict';

const createAnimatedComponent = require('../createAnimatedComponent');
const createAnimatedComponent = require('../createAnimatedComponent').default;
Copy link
Collaborator Author

@gabrieldonadel gabrieldonadel Aug 31, 2022

Choose a reason for hiding this comment

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

Using import here also breaks jest

const createAnimatedComponentInjection = require('../createAnimatedComponentInjection');
const React = require('react');
import * as React from 'react';

function injected<TProps: {...}, TInstance>(
Component: React.AbstractComponent<TProps, TInstance>,
Expand Down
6 changes: 2 additions & 4 deletions Libraries/Animated/animations/Animation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

'use strict';

const NativeAnimatedHelper = require('../NativeAnimatedHelper');
import NativeAnimatedHelper from '../NativeAnimatedHelper';
import type {PlatformConfig} from '../AnimatedPlatformConfig';
import type AnimatedValue from '../nodes/AnimatedValue';

Expand All @@ -30,7 +30,7 @@ let startNativeAnimationNextId = 1;
// Important note: start() and stop() will only be called at most once.
// Once an animation has been stopped or finished its course, it will
// not be reused.
class Animation {
export default class Animation {
__active: boolean;
__isInteraction: boolean;
__nativeId: number;
Expand Down Expand Up @@ -85,5 +85,3 @@ class Animation {
}
}
}

module.exports = Animation;
10 changes: 4 additions & 6 deletions Libraries/Animated/animations/DecayAnimation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@

'use strict';

const Animation = require('./Animation');
import Animation from './Animation';

const {shouldUseNativeDriver} = require('../NativeAnimatedHelper');
import NativeAnimatedHelper from '../NativeAnimatedHelper';

import type {PlatformConfig} from '../AnimatedPlatformConfig';
import type AnimatedValue from '../nodes/AnimatedValue';
Expand All @@ -36,7 +36,7 @@ export type DecayAnimationConfigSingle = {
deceleration?: number,
};

class DecayAnimation extends Animation {
export default class DecayAnimation extends Animation {
_startTime: number;
_lastValue: number;
_fromValue: number;
Expand All @@ -51,7 +51,7 @@ class DecayAnimation extends Animation {
super();
this._deceleration = config.deceleration ?? 0.998;
this._velocity = config.velocity;
this._useNativeDriver = shouldUseNativeDriver(config);
this._useNativeDriver = NativeAnimatedHelper.shouldUseNativeDriver(config);
this._platformConfig = config.platformConfig;
this.__isInteraction = config.isInteraction ?? !this._useNativeDriver;
this.__iterations = config.iterations ?? 1;
Expand Down Expand Up @@ -123,5 +123,3 @@ class DecayAnimation extends Animation {
this.__debouncedOnEnd({finished: false});
}
}

module.exports = DecayAnimation;
Loading