Add in Latest Code From React Native #64

browniefed · 2017-04-11T17:41:59Z

Ported latest react native code over. So much has changed. All things were moved into single file.
Added back in AnimatedTemplate (need to port this to RN)
Subscriptions on interpolations. Are added as well ( should port this to RN )

shrug Add back inject Rework to use injectable currents Fix up exports NativeAnimatedHelper

browniefed · 2017-04-11T17:48:43Z

Ah shit need to move back injectables

Edit: Added it back

necolas · 2017-04-11T18:24:05Z

package.json

@@ -2,7 +2,7 @@
  "name": "animated",
  "version": "0.2.0",
  "description": "Declarative Animations Library for React and React Native",
-  "main": "lib/index.js",
+  "main": "lib/targets/react-dom",


Why this change? Would react-native and react-native-web I need to import from animated/lib/targets/react-native to get this working?

Ah sorry. The index.js doesn't exist anymore.
Now there is just AnimatedImplementation and 2 targets.

Do you have an idea on how we can swap main entry that when running on RN it points to lib/targets/react-native and on web lib/targets/react-native.

I know the react-native packager will look at react-native key in package.json. However not sure how that works inside of a library.

All in all. Do you know of a way I can support you on this? Add back index.js that just exports the RN implementation? Then also exports web?

browniefed · 2017-04-11T18:58:01Z

@necolas Updated main to point to the RN target export.
Additionally added

"browser": {
    "NativeModules": false,
    "NativeEventEmitter": false,
    "ReactNative": false,
    "ViewStylePropTypes": false
  },

To the package.json to maintain imports but just disable them for browser.

Let me know if that will break react-native-web

EDIT: Ah forgot this would only effect interactive-docs and not just the babel config.

Should investigate a real build system for the web version.

vjeux

I don't have the time to review this but I'm happy if it's still working!

browniefed · 2017-04-11T19:55:38Z

@vjeux no worries, I figured. Interactive docs still build and run. Might actually go rebuild them to not have embedded babel on the page.

necolas · 2017-04-11T20:05:22Z

I have no idea what the browser map is for, or what it will do. The currently published version works for RNW - https://github.com/necolas/react-native-web/blob/master/src/apis/Animated/index.js. Why can't you retain the index.js file?

browniefed · 2017-04-11T20:19:43Z

@necolas browser was just for the browserify build that generates the documentation. Need to probably drop the Native* stuff since it's just used for RN and or add a real build process vs just babel.

My ultimate goal with this is to bring this package and the RN implementation inline with each other for easier upgrading, and eventually RN depending on this library.

So there is no index.js in the https://github.com/facebook/react-native/tree/master/Libraries/Animated/src folder anymore.

So I removed it in favor of selecting specific targets. In the future the RN would point Animated => animated/lib/targets/react-native and for web build would swap and point to lib/targets/react-web.

You are seemingly just pulling in the Animated lib without the injectables, but I'm not sure what you're doing to fix styles appropriately.

The https://github.com/animatedjs/animated/blob/master/src/targets/react-dom.js#L16 is what injects and overrides how styles are applied to DOM elements.

necolas · 2017-04-11T21:03:07Z

Yeah I don't want to use the react-dom target because react-native-web handles all that already. I probably need to be able to define my own "target" (and many libs want to do that too, e.g., ReactXP), especially as the way you're importing ScrollView in this patch will break as soon as I switch to ES6 module exports - which are important for tree-shaking web builds. The fact that RN doesn't have an index seems incidental if you're positioning this library as a RN dependency - could RN define its own target like I do in RNW?

browniefed · 2017-04-11T21:07:44Z

@necolas I will remove the ScrollView reference in this, as that was just react-native doing it's lazy loading and this is mostly for targeting web at the moment.

Ideally this would be using ES6 imports but for some reason RN core hasn't switched over yet.

Yes the idea would be that we could tell the packager via @providesModule Animated and do this inside of the react-native target. RN rarely uses relative paths and instead uses an @providesModule annotation to map named imports to arbitrary files located where ever. ( https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/Animated.js#L9 )

Either way a lot more work needs to be put into this before merging and I'll be sure to not break react-native-web.

necolas · 2017-04-11T21:57:51Z

It looks like a potential RN diff would change https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/Animated.js#L15

var AnimatedImplementation = require('AnimatedImplementation');

to

var AnimatedImplementation = require('animated');

…if this package continued to export the current index.js file rather than pre-defined targets. The downside of predefined targets is that you're making assumptions about what a DOM target needs. The "react-native" export is not really saying anything about the target platform, but the primitives used - those primitives are also available on web and other platforms. It seems like this change would also be inviting RN/RNW/RDOM consumers to make changes to this package if they want to hang more components off the target export (like when RN added ScrollView), rather than leaving those details to the renderers.

browniefed · 2017-04-11T22:08:38Z

Yeah I understand that. Why don't I just add back index.js.
It will require lib/targets/react-native and re-export it.
Also remove the ScrollView export.

react-native-web will continue to work as expected, as well as all other code that links directly to target for web.

Then we can solve the target issue later rather than in this upgrade.

browniefed · 2017-04-11T22:16:57Z

All patched up. Let me know what you think.

lelandrichardson · 2017-04-12T00:18:35Z

package.json

@@ -28,8 +29,7 @@
  },
  "homepage": "https://github.com/animatedjs/animated#readme",
  "dependencies": {
-    "invariant": "^2.2.0",
-    "normalize-css-color": "^1.0.1"


seems like we could continue relying on this instead of copying over the implementation

lelandrichardson · 2017-04-12T00:19:05Z

package.json

@@ -45,7 +45,7 @@
    "babel-preset-react": "^6.5.0",
    "babel-preset-react-native": "^1.4.0",
    "browserify": "^13.0.0",
-    "react": "^15.4.0",
-    "react-dom": "^15.4.0"
+    "react": "^15.5.3",


did something change that made this library incompatible with react 15.4?

lelandrichardson · 2017-04-12T00:22:05Z

In general I'm a little bit bummed we are moving back to a flat file for this library. It's much harder to maintain. If we want to make it easier to stay in sync with RN, it might be better to submit a PR to RN core splitting it up into multiple files?

I think we want to keep an index.js or some other entry file that exports the animated implementation without anything injected or without any components created, so libraries like RNW can create their own "version" of it. (React Native and react-dom could also potentiallydo the same).

Thanks for working on this! let's be careful though. I'd take a look through the commit history of this repo to make sure we aren't losing valuable bug fixes that have happened since the original "sync" from RN.

browniefed · 2017-04-12T04:17:27Z

I wasn't sure if you had split it out into separate files or if rn consolidated it all.

I prefer separate files and I think best move is go submit a PR to RN like you said, and split out to files.

I'm not sure why it's all in 1 big file honestly.

Once all separate doing a diff should expose any potential areas that are different and or bug fixed to preserve.

EDIT: Would it be easier to publish animated-web and then animated-native?

lelandrichardson · 2017-04-13T18:42:57Z

EDIT: Would it be easier to publish animated-web and then animated-native?

@browniefed I'd like to stay away from that if we can!

jbovenschen · 2017-08-17T19:56:34Z

@browniefed Any progress on this? Or is there anything I can help you with?

browniefed · 2017-08-17T20:01:43Z

Progress is being made in the RN repo. Janic has split everything out into it's separate files.

Once that is accepted this can continue on.

browniefed · 2017-08-17T20:01:58Z

Closing this in favor of a different PR later.

Add in new code

ea17932

shrug Add back inject Rework to use injectable currents Fix up exports NativeAnimatedHelper

browniefed requested a review from lelandrichardson April 11, 2017 17:41

Clean up more, reexport injects

1fe7dd0

browniefed mentioned this pull request Apr 11, 2017

missing Animated.diffClamp #57

Open

necolas reviewed Apr 11, 2017

View reviewed changes

Jason Brown added 3 commits April 11, 2017 11:41

Add back listeners on interpolation

1440969

Mock out native modules with browser field

6162b9e

Use browser to disable modules

c02f6d0

browniefed requested a review from vjeux April 11, 2017 19:02

vjeux approved these changes Apr 11, 2017

View reviewed changes

Maintain previous exporting

48dff4d

Remove scrollview

e9e96c9

lelandrichardson reviewed Apr 12, 2017

View reviewed changes

browniefed closed this Aug 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in Latest Code From React Native #64

Add in Latest Code From React Native #64

browniefed commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017 •

edited

necolas Apr 11, 2017

browniefed Apr 11, 2017

browniefed commented Apr 11, 2017 •

edited

vjeux left a comment

browniefed commented Apr 11, 2017

necolas commented Apr 11, 2017

browniefed commented Apr 11, 2017 •

edited

necolas commented Apr 11, 2017

browniefed commented Apr 11, 2017 •

edited

necolas commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017

browniefed commented Apr 11, 2017

lelandrichardson Apr 12, 2017

lelandrichardson Apr 12, 2017

lelandrichardson commented Apr 12, 2017

browniefed commented Apr 12, 2017 •

edited

lelandrichardson commented Apr 13, 2017

jbovenschen commented Aug 17, 2017

browniefed commented Aug 17, 2017

browniefed commented Aug 17, 2017

Add in Latest Code From React Native #64

Add in Latest Code From React Native #64

Conversation

browniefed commented Apr 11, 2017 • edited

browniefed commented Apr 11, 2017 • edited

necolas Apr 11, 2017

Choose a reason for hiding this comment

browniefed Apr 11, 2017

Choose a reason for hiding this comment

browniefed commented Apr 11, 2017 • edited

vjeux left a comment

Choose a reason for hiding this comment

browniefed commented Apr 11, 2017

necolas commented Apr 11, 2017

browniefed commented Apr 11, 2017 • edited

necolas commented Apr 11, 2017

browniefed commented Apr 11, 2017 • edited

necolas commented Apr 11, 2017 • edited

browniefed commented Apr 11, 2017

browniefed commented Apr 11, 2017

lelandrichardson Apr 12, 2017

Choose a reason for hiding this comment

lelandrichardson Apr 12, 2017

Choose a reason for hiding this comment

lelandrichardson commented Apr 12, 2017

browniefed commented Apr 12, 2017 • edited

lelandrichardson commented Apr 13, 2017

jbovenschen commented Aug 17, 2017

browniefed commented Aug 17, 2017

browniefed commented Aug 17, 2017

browniefed commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017 •

edited

browniefed commented Apr 11, 2017 •

edited

necolas commented Apr 11, 2017 •

edited

browniefed commented Apr 12, 2017 •

edited