-
Notifications
You must be signed in to change notification settings - Fork 108
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.
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
Move to injection API #58
Conversation
src/injection/react-native-web.js
Outdated
Touchable, | ||
Dimensions, | ||
} = require('react-native-web'); | ||
const Easing = require('animated/lib/Easing'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also exported from RNW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know!
@@ -0,0 +1,38 @@ | |||
const ReactPrimitives = require('../ReactPrimitives'); | |||
|
|||
const PixelRatio = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this API implemented by each target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like PixelRatio
can be implemented purely as a function of Dimensions
in most cases, so i'm leaving that as an option for targets. i might change this to override it if it's provided though
* }, | ||
* ``` | ||
*/ | ||
const Touchable = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing a custom touchable makes it a bit harder for anyone to use react-primitives instead of react-native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been playing around a lot with the react-native touchables and I don't know if they're up to scratch, in that I can't extend them beyond a change in colour and opacity.
Those variations are OK, but often I'm looking for a more subtle controls: Touch up, and touch down with an animatable transition of my choosing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean - but I think that Touchable
(considering renaming to Pressable
) is actually a better, more general implementation that allows you to create more things with it (including all of the Touchable* implementations that RN exports).
* ``` | ||
*/ | ||
const Touchable = ( | ||
Animated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While working on RNW people reported that using animated in the touchables was a performance issue in some circumstances so I switched to CSS transitions. Animated itself is quite large and using it for animations means you're on the main thread a lot. If animated could rely on web APIs where possible (e.g. element.animate) we'd see better performance and smaller bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good feedback. I'll consider removing animated from this API, but i do actually think it's pretty critical :/
For the runtime performance issue, i'm planning on implementing TouchableOpacity/TouchableHighlight in terms of Touchable
, but we could have an optimized .web.js
implementation that uses CSS transitions. We can't use CSS transitions in the same level of generality as we can do with Animated.Value
for more complex cases though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm going to try and get this merged as is, but we should have a broader discussion about animated, especially as it pertains to size, as we want to try and make RP as small as possible especially on web
Thanks, the injection API looks worth a shot. Left some comments on the patch, mainly about including animated in the API |
a7fc1f6
to
812c0f0
Compare
to: @necolas @jongold
This PR changes react-primitives to be effectively just an interface or proxy to the target implementation. Libraries and modules can import react-primitives directly without assuming any sort of platform target. This allows for new platforms / targets to be created and experimented on without living in this repo, which I view as an important goal.
The main purpose of
react-primitves
will then be:The main APIs of react-primitives ends up being the following:
Note that i've added
Dimensions
andPixelRatio
here because I think these APIs fall into the purview of primitives... they are something that are inherent to the platform, and I think building certain things will require those APIs. From what i've seen,Dimensions
is a pretty highly used API from RN (andPixelRatio
is closely related, so makes sense to come with it).Note that
Touchable
is not the same as react-native'sTouchable
, and is a touchable-like interface that is coupled withAnimated
. From this API, you can make things likeTouchableOpacity
andTouchableHighlight
(and much more!)Then, there is an
inject
API, where you pass in these APIs and they will then be exported from the react-primitives module.Pros:
Cons:
Default Injections:
One problem with the "injection" API is that this would now mean that if some random library wanted to start depending on
react-primitives
where they currently just depend onreact-dom
, they would have to make it a breaking change since now the consumers of the library need to know aboutreact-primitives
and how to inject a target. This is obviously not ideal.One way to fix this is to have the most common platforms set up to inject automatically. We can have this module have platform extensions in their entry points that do the injection. For example, we have:
This would mean that a library that was previously intended for the RN community or the web community would "just work" if moving over to primitives and could do so without breaking their previous API. The RN packager will automatically choose the
ios.js
orandroid.js
target and would set itself up automatically with no extra user steps.In this case we are choosing "web" as the default in that any bundler that isn't using platform extensions specifically is going to fall back to
index.js
where we will be injecting the web target. As a result,react-native-web
will end up being our explicit dependency.The downside here is that this is pretty gross and implicit. If someone has a bundler that is using
.ios.js
for some reason, but NOT usingreact-native
, then this project will technically be making arequire
call to a module that isn't listed in it's package.json. Gross, but I'm thinking this may be a net-positive trade-off considering all of the angles.