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

gabrieldonadel · 2022-08-31T00:44:18Z

Summary

This PR refactors the Animated directory to use ESModule imports/exports instead of using a mixture of the 2 module formats, as requested on #34425.

Changelog

[Internal] [Changed] - Convert all files in the Animated directory to use ESModule imports/exports

Test Plan

This doesn't really add or modify any existing features so checking if CI passes should be enough

analysis-bot · 2022-08-31T01:26:49Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: b2452ab
Branch: main

analysis-bot · 2022-08-31T01:29:59Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,641,961	+380
android	hermes	armeabi-v7a	7,054,187	+385
android	hermes	x86	7,943,538	+385
android	hermes	x86_64	7,915,532	+378
android	jsc	arm64-v8a	9,515,017	+295
android	jsc	armeabi-v7a	8,290,641	+310
android	jsc	x86	9,454,346	+295
android	jsc	x86_64	10,045,412	+293

Base commit: b2452ab
Branch: main

javache

Looks like CI tests are still failing and there's a lint warning.

javache · 2022-08-31T09:00:26Z

Libraries/Animated/Animated.js

+  ScrollView: AnimatedScrollView,
+  SectionList: AnimatedSectionList,
+  Text: AnimatedText,
+  View: AnimatedView,
  ...Animated,
 };


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)

Got it, I'll revert this

@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 ?

@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.

@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 ;)

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)

Libraries/Animated/NativeAnimatedHelper.js

gabrieldonadel · 2022-08-31T15:09:14Z

Libraries/Animated/__tests__/Animated-test.js

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

-let Animated = require('../Animated');
+let Animated = require('../Animated').default;


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

gabrieldonadel · 2022-08-31T15:11:41Z

Libraries/Animated/__tests__/AnimatedNative-test.js

+const Animated = require('../Animated').default;
+const NativeAnimatedHelper = require('../NativeAnimatedHelper').default;


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

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

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

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.

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

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.

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

gabrieldonadel · 2022-08-31T15:13:47Z

Libraries/Animated/__tests__/createAnimatedComponentInjection-test.js

@@ -11,9 +11,9 @@

 'use strict';

-const createAnimatedComponent = require('../createAnimatedComponent');
+const createAnimatedComponent = require('../createAnimatedComponent').default;


Using import here also breaks jest

Libraries/Animated/nodes/AnimatedProps.js

Libraries/Animated/NativeAnimatedHelper.js

facebook-github-bot · 2022-09-02T09:27:49Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2022-09-09T12:30:10Z

This is currently blocked on merging internally, since it breaks the flow-type export of Animated.Numeric

gabrieldonadel · 2022-09-09T12:33:49Z

This is currently blocked on merging internally, since it breaks the flow-type export of Animated.Numeric

Is there something that I can help with?

react-native-bot · 2022-09-20T17:15:05Z

This pull request was successfully merged by @gabrieldonadel in f63d4e7.

^{When will my fix make it into a release? | Upcoming Releases}

…acebook#34539) Summary: This PR refactors the Animated directory to use ESModule imports/exports instead of using a mixture of the 2 module formats, as requested on facebook#34425. ## Changelog [Internal] [Changed] - Convert all files in the Animated directory to use ESModule imports/exports Pull Request resolved: facebook#34539 Test Plan: This doesn't really add or modify any existing features so checking if CI passes should be enough Reviewed By: yungsters Differential Revision: D39235720 Pulled By: yungsters fbshipit-source-id: 84b4c0a71dc9fca1ab7053263f1cf7c336df58c1

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 31, 2022

gabrieldonadel force-pushed the refactor/animated-directory-to-use-es6 branch from 9ed091b to ea74740 Compare August 31, 2022 01:06

gabrieldonadel force-pushed the refactor/animated-directory-to-use-es6 branch from ea74740 to be02361 Compare August 31, 2022 01:29

javache requested changes Aug 31, 2022

View reviewed changes

gabrieldonadel force-pushed the refactor/animated-directory-to-use-es6 branch from be02361 to f24ae20 Compare August 31, 2022 15:02

gabrieldonadel commented Aug 31, 2022

View reviewed changes

Libraries/Animated/nodes/AnimatedProps.js Outdated Show resolved Hide resolved

gabrieldonadel requested review from javache and TMaszko and removed request for javache and TMaszko August 31, 2022 15:14

necolas mentioned this pull request Aug 31, 2022

[w3c] ☂️ Web Styles (Part 1) umbrella issue #34425

Open

74 tasks

javache reviewed Sep 1, 2022

View reviewed changes

Libraries/Animated/NativeAnimatedHelper.js Show resolved Hide resolved

gabrieldonadel requested a review from javache September 1, 2022 15:33

gabrieldonadel force-pushed the refactor/animated-directory-to-use-es6 branch from a2155bb to f99174d Compare September 1, 2022 16:24

javache approved these changes Sep 2, 2022

View reviewed changes

gabrieldonadel added 3 commits September 8, 2022 15:59

refactor: Convert Animated directory to use ESModule imports/exports

67c84c6

chore: Update NativeAnimatedHelper imports

c300338

chore: Update AnimatedProps imports

f45fb04

gabrieldonadel force-pushed the refactor/animated-directory-to-use-es6 branch from f99174d to f45fb04 Compare September 8, 2022 18:59

facebook-github-bot closed this in f63d4e7 Sep 20, 2022

react-native-bot added the Merged This PR has been merged. label Sep 20, 2022

gabrieldonadel deleted the refactor/animated-directory-to-use-es6 branch September 20, 2022 17:19

necolas mentioned this pull request Nov 8, 2022

☂️ npm packages for react-native-web #35263

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gabrieldonadel commented Aug 31, 2022

analysis-bot commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

javache left a comment

javache Aug 31, 2022

gabrieldonadel Aug 31, 2022

TMaszko Aug 31, 2022

gabrieldonadel Aug 31, 2022

javache Sep 1, 2022

TMaszko Sep 1, 2022 •

edited

Loading

javache Sep 2, 2022

gabrieldonadel Aug 31, 2022

gabrieldonadel Aug 31, 2022

gabrieldonadel Aug 31, 2022

javache Sep 1, 2022

TMaszko Sep 1, 2022

gabrieldonadel Sep 1, 2022

necolas Sep 1, 2022

TMaszko Sep 1, 2022

gabrieldonadel Aug 31, 2022

facebook-github-bot commented Sep 2, 2022

javache commented Sep 9, 2022

gabrieldonadel commented Sep 9, 2022

react-native-bot commented Sep 20, 2022

		const Animated = require('../Animated').default;
		const NativeAnimatedHelper = require('../NativeAnimatedHelper').default;

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

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

Conversation

gabrieldonadel commented Aug 31, 2022

Summary

Changelog

Test Plan

analysis-bot commented Aug 31, 2022 • edited Loading

analysis-bot commented Aug 31, 2022 • edited Loading

javache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TMaszko Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 2, 2022

javache commented Sep 9, 2022

gabrieldonadel commented Sep 9, 2022

react-native-bot commented Sep 20, 2022

analysis-bot commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

TMaszko Sep 1, 2022 •

edited

Loading