-
Notifications
You must be signed in to change notification settings - Fork 928
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
[WIP] Add support for React Native as a target #265
Conversation
src/downshift.js
Outdated
@@ -621,7 +630,7 @@ class Downshift extends Component { | |||
this.internalSetState({ | |||
type: Downshift.stateChangeTypes.changeInput, | |||
isOpen: true, | |||
inputValue: event.target.value, | |||
inputValue: isReactNative() ? event : event.target.value, |
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 seems a bit odd, since the onChangeText
prop on React Native returns just the text as a string, not an object. Is there a better way to go about this?
src/downshift.js
Outdated
setA11yStatus(status) | ||
// TODO: make this work. | ||
if (!isReactNative()) { | ||
setA11yStatus(status) |
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 looks like setA11yStatus
calls a bunch of DOM-specific APIs, which don't exist on React Native. Should we fork the logic and send this a11y status elsewhere?
src/utils.js
Outdated
* @return {Boolean} whether or not the platform is React native | ||
*/ | ||
function isReactNative() { | ||
return navigator != null && navigator.product === 'ReactNative' |
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.
Can this be preval
ed?
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 think the answer is no, since this needs to exist at runtime. Maybe there's a better alternative to use here that could help us cut down on some function calls.
Alright, spent some time playing around with this today, and it looks like there's some corners to be cut to get this to work on React Native. There's a few functions that are DOM-specific, like With regards to @gricard Had mentioned in #185 (comment) that there are a few accessibility props that we could pass to input, button, and item views, but I omitted those for now, since they're I'd love to find a cleaner way to get this in without littering Additionally, should I add in a React Native test suite as well? I believe this would just involve adding |
Oh! Additionally, there's a sample project here: https://github.com/eliperkins/DownshiftNative |
src/downshift.js
Outdated
const node = this.getItemNodeFromIndex(this.getState().highlightedIndex) | ||
const rootNode = this._rootNode | ||
scrollIntoView(node, rootNode) | ||
// TODO: this whole method is DOM specific... |
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.
Should we just omit this logic here? This is specific to DOM and to get this to work with React Native, we'd have to force consumers to use a ScrollView
and pass us the ref.
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.
Yeah, we'll probably want some different logic here for React Native
One last question, should we omit a call to |
Nice! Thanks for picking up the torch there. I got wicked sidetracked. 👍 |
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 didn't have a whole lot of time to review, but left a few comments.
I think in general I'd prefer if we could do something similar with this that we do with preact. Specifically, that we have a separate build version of the library for the react native version. That way folks can use that build and people using the react build don't have to suffer a perf penalty for all the logic around react native.
This is going to require a fair amount of work. But I'm convinced it'll be better for the community if we do it as part of downshift rather than building another project... Maybe I'm wrong... But I don't think so.
src/downshift.js
Outdated
@@ -172,6 +173,13 @@ class Downshift extends Component { | |||
} | |||
|
|||
getItemNodeFromIndex = index => { | |||
if (isReactNative()) { | |||
throw new Error( |
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.
We don't need to worry about throwing an error because getItemNodeFromIndex
is not publicly exposed so this should never be called :)
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.
Heh, good call. This was part of my debugging earlier to figure out what codepath was being followed to call this. I'll remove it!
src/downshift.js
Outdated
const node = this.getItemNodeFromIndex(this.getState().highlightedIndex) | ||
const rootNode = this._rootNode | ||
scrollIntoView(node, rootNode) | ||
// TODO: this whole method is DOM specific... |
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.
Yeah, we'll probably want some different logic here for React Native
Oh, also, THANK YOU SO MUCH FOR WORKING ON THIS! 🎉 |
@kentcdodds sounds good! I think that approach makes sense. It looks like the Preact build pipeline is pretty embedded into |
Let's keep it separate. No need to abstract it away until we know what it looks like 😉 I do expect it to be pretty similar though. Some environment variable we use with preval. We could probably make a rollup.config.rn.js file for this |
I took a quick pass at this, it seems like I've added a new build script (that still needs to get hooked into the normal Unfortunately, it looks like our calls to I hope I understood your comment about creating a new target here, and took the right approach! Feedback is more than welcome. 😄 |
Oh man, I didn't realize the |
Yeah... I think that the babel plugin we're using for dead code elimination doesn't handle Judging from this UglifyJS doesn't actually handle this very well either. So perhaps a better solution would be to solve the issues with integrating |
Thanks so much for all your work on this. It's awesome! |
package.json
Outdated
@@ -9,6 +9,7 @@ | |||
"scripts": { | |||
"add-contributor": "kcd-scripts contributors add", | |||
"build": "kcd-scripts build --bundle --p-react", | |||
"build:react-native": "BUILD_REACT_NATIVE=true BUILD_FILENAME_PREFIX=react-native kcd-scripts build --bundle", |
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 not sure the best way to combine this build step with the normal build
without clobbering some of the niceties of kcd-scripts build
(by using &&
to run both commands).
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'd just have build:web
and a build:native
scripts and then a build
script that just runs them both: npm run build:web --silent && npm run build:native --silent
👌
src/utils.js
Outdated
@@ -224,6 +226,17 @@ function isDOMElement(element) { | |||
} | |||
} | |||
|
|||
/** | |||
* React Native sets `navigator.product` to a constant `ReactNative`. |
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 comment is no longer valid and should be removed.
Alright, I kinda tackled this with as minimal destruction as I could. I ended committing a Additionally, I appeased I think the biggest open question I have is this comment: #265 (comment). I'm not sure the best way to include this in the current build script without extending |
cf5a780
to
dc3611e
Compare
Codecov Report
@@ Coverage Diff @@
## master #265 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 4 4
Lines 324 327 +3
Branches 80 84 +4
=====================================
+ Hits 324 327 +3
Continue to review full report at Codecov.
|
Hey @kentcdodds! Happy New Year! 🎊 Any chance you could take another peek at this at some point in the next few days? I'd love a little more advice regarding two things:
Cheers! |
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.
Things are looking great! Just a few comments.
Thanks again for working on this!
package.json
Outdated
@@ -9,6 +9,7 @@ | |||
"scripts": { | |||
"add-contributor": "kcd-scripts contributors add", | |||
"build": "kcd-scripts build --bundle --p-react", | |||
"build:react-native": "BUILD_REACT_NATIVE=true BUILD_FILENAME_PREFIX=react-native kcd-scripts build --bundle", |
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'd just have build:web
and a build:native
scripts and then a build
script that just runs them both: npm run build:web --silent && npm run build:native --silent
👌
src/utils.js
Outdated
// Disabling coverage here is necessary due to a weird deal with | ||
// babel-plugin-istanbul + preval.macro. No idea... | ||
/* istanbul ignore next (preact) */ | ||
const isPreact = preval`module.exports = process.env.BUILD_PREACT === 'true'` |
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 think that these will actually need to be inlined as they were before to make sure the code is removed properly. I realize it's not as clean, but it'll work better.
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 PR got me interested and I agree - better to have those branches DCE-ed.
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.
Hm, I'm not sure what you mean by making sure the code is removed properly, @kentcdodds, nor do I know what "DCE-ed" means @Andarist. Can you clarify?
If I understand @kentcdodds, he's referring to the code being removed when the production minified bundle is created by Rollup, which seems like it is, according to my tests. Here are some examples:
dist/downshift.umd.min.js
vs preact/downshift.umd.min.js
Note that this constant is different, using the isPreact
constant pulled in from the utils
module.
dist/downshift.umd.min.js
this.scrollHighlightedItemIntoView=function(){!function(e,t){var n=S(e,t);if(null!==n){var o=getComputedStyle(n),i=n.getBoundingClientRect(), /* ...rest of scrollIntoView impl */
Notes that there is no runtime check to the isReactNative
constant in this function body.
I believe since these are inlined as constants of true
or false
, Rollup is able to minify that codepath away. If this isn't what these comments are talking about, I'd love some help here!
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.
Thanks! I'm less concerned about the umd
builds and more concerned about all the other ones. Could you run the builds and see if Rollup was able to eliminate those? If not, could you try to inline the preval usage and see if Rollup can eliminate it that way. I know it's not pretty, but we've gotta work with the tools we have.
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.
Ah, gotcha. Rollup does not eliminate them there. I'll add the preval
s back in, inline!
Do you have more reading for me to learn more about why someone might not use the umd.min
version of the build so I can understand the benefit of optimizing those builds?
src/downshift.js
Outdated
@@ -661,7 +674,9 @@ class Downshift extends Component { | |||
this.internalSetState({ | |||
type: Downshift.stateChangeTypes.changeInput, | |||
isOpen: true, | |||
inputValue: event.target.value, | |||
inputValue: isReactNative | |||
? /* istanbul ignore next (react-native) */ event |
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.
Interesting! In react native the event
is considered the inputValue
? That's a surprise... Is it really a string? The inputValue
should be a string that's the value of the input. Other parts of the code make that assumption. Just want to clarify that in react native event
is a string that's the value of the input. If not, then we need to get the actual string value.
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.
Yup! This specific callback, onChangeText
, takes a function that is called with the text as a string, that's it.
Other callbacks on TextInput
that are more generic for the input itself (like onBlur
or onSubmitEditing
) return an event as a Proxy
in the shape of { nativeEvent: { text: string } }
, however, this specific callback just gives us a string
.
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 don't think there is a need to produce module
entry for react-native. From what I know metro bundler doesn't understand ESM, only CJS.
You could also remove need for separate react-native
directory which enforces importing from downshift/react-native
and not directly from downshift
.
There are 2 ways for doing this:
- put "react-native": "dist/downshift.native.js" in your root package.json
- change your "main" in root package.json from
dist/downshift.cjs.js
todist/downshift
. Let node and company add.js
extension automatically. Create your react-native bundle at<pkg.root>/dist/downshift.native.js
and let Metro Bundler choose this automatically over regular.js
extension.
Both options will allow for import downshift from 'downshift'
in React Native
Thanks! I prefer option 1 👍 |
It would probably be good to add support for this in your |
Maybe so... But let's get it working here first 😉 |
Ah! Thanks so much for the feedback @Andarist!!! I apologize I haven't gotten around to wrapping this up yet, it's been super busy for me with work and life. I should be able to spend an hour or two on this tomorrow though! ❤️ |
The build command now builds for both web and native targets
This will prevent us from smashing against different build targets by sharing a build directory. This also uses the "react-native" key in the package.json to tell Metro Bundler where to get the module from.
This will allow Rollup to optimize builds for UMD, ESM and CJS, without minification.
I can finish this up. I'm not a maintainer of this package though, so you'd have to invite me to your fork I guess. |
const rootNode = this._rootNode | ||
scrollIntoView(node, rootNode) | ||
/* istanbul ignore else (react-native) */ | ||
if (preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`) { |
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 dont think preval
is actually needed here, rollup should handle this just fine with rollup-plugin-replace
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 like that idea. Let's add a feature to kcd-scripts
and then upgrade this to use that feature.
I think we'll want to configure that somehow though (CLI args?) because there are some process.env.*
variables that we want to leave as-is.
221d422
to
1d35486
Compare
@Andarist, I've added you as a collaborator to downshift 👍 Thanks for all your help so far. Here's the normal thing I show peopel when I add them: Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the |
Cool, it doesnt seem there is much to be done. I will wrap this up tomorrow (its midnight here already) and ping you for a review. |
Super duper! |
@kentcdodds pretty much done, I've tweaked the build setup and it was the only thing I was looking at - I didn't actually test this on RN (functionality-wise). Things I've noticed while doing this:
|
package.json
Outdated
"module": "dist/downshift.esm.js", | ||
"typings": "typings/index.d.ts", | ||
"scripts": { | ||
"add-contributor": "kcd-scripts contributors add", | ||
"build": "npm run build:web --silent && npm run build:native --silent", | ||
"build:web": "kcd-scripts build --bundle --p-react", | ||
"build:native": "BUILD_REACT_NATIVE=true BUILD_FILENAME_PREFIX=react-native kcd-scripts build --out-dir native", | ||
"build:native": "BUILD_REACT_NATIVE=true BUILD_FILENAME_SUFFIX=.native kcd-scripts build --bundle cjs --no-clean", |
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.
What's the rationale around including the --no-clean
flag here?
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.
build:web
are and build:native
are ran sequentially, kcd-scripts build --bundle
removes the target directory, so its removed during build:web
and I dont want to remove what was just built by it by running build:native
the goal for me was to not introduce a separate directory and Ive wanted to output react-native version into dist
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.
What do you think of adding --no-clean
only to the build
command, rather than build:native
? That way, the build:native
command can be run safely, clearing out the dist
directory, while the build
command would be able to handle the sequential case like you mentioned.
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.
that makes sense
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.
@eliperkins changed
package.json
Outdated
@@ -29,8 +29,7 @@ | |||
"files": [ | |||
"dist", | |||
"typings", | |||
"preact", | |||
"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.
Thanks for tidying this up for me!
d06f84b
to
fb3e90c
Compare
@eliperkins By DCE-ed I've meant dead code eliminated. It's actually UglifyJS which does it and not rollup, let's look on a sample code after its gets built with rollup: {
key: 'input_getOnChangeKey',
value: function input_getOnChangeKey() {
/* istanbul ignore next (preact) */
if (isPreact) {
return 'onInput';
/* istanbul ignore next (react-native) */
} else if (isReactNative) {
return 'onChangeText';
} else {
return 'onChange';
}
}
}, We can that it is not eliminated here (without minification). I've removed var onChangeKey = void 0;
/* istanbul ignore next (preact) */
onChangeKey = 'onChange'; Still without minification. It seems that rollup also does some dead code elimination on its own, but has less superior algorithm. When it comes to UglifyJS there are also many versions out there and there are also many settings that one can specify for it. Dunno if such constants can be easily inlined in every case if they are not local. I was under the impression that many times it fails and this wouldn't be properly dead code eliminated. For sure flat bundling helps here, because if it would be split across files uglifyJS wouldnt be able to eliminate this, that is something im 100% sure. But anyway as we cannot be sure what kind of minification settings or uglifyJS version consumers' are using it's just better to put those constant conditions as local as possible as it's always easier to analyze local situation than more global one and we can make it easier for other tools (also other minifiers out there) to handle this properly. |
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.
Looking great!
package.json
Outdated
"build": "kcd-scripts build --bundle --p-react", | ||
"build": "npm run build:web --silent && npm run build:native --silent -- --no-clean", | ||
"build:web": "kcd-scripts build --bundle --p-react", | ||
"build:native": "BUILD_REACT_NATIVE=true BUILD_FILENAME_SUFFIX=.native kcd-scripts build --bundle cjs", |
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.
Let's add a devDependency on cross-env
and use that here so this works on windows machines as well (considering the setup
script will run validate
which will run the build. I want to make sure that script always works on windows.
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.
done
const rootNode = this._rootNode | ||
scrollIntoView(node, rootNode) | ||
/* istanbul ignore else (react-native) */ | ||
if (preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`) { |
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 like that idea. Let's add a feature to kcd-scripts
and then upgrade this to use that feature.
I think we'll want to configure that somehow though (CLI args?) because there are some process.env.*
variables that we want to leave as-is.
I think that's probably a left-over mistake or something. Feel free to remove it as part of this PR! Thanks! |
I can do that, but this shouldnt hold off this PR
Not sure what variables you'd like to leave as is, I'd just take all
gonna send separate PR for this |
The thing is we want to use I imagine we'll want to use it in the future as well. Currently the UMD bundle replaces that ( I agree that should not hold up this PR. Is this ready to merge then? |
I think when we merge this we'll merge this with a note that it's experimental and could break. Would be good to get feedback on it before it's official. |
damn, I totally forgot about
from my POV - yes
Sure thing. |
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.
Awesome. I'm merging this now.
Here's what we need before react-native support becomes stable:
- A committed maintainer (I don't use RN, so I can't maintain it). I think at least 2 of you are committed here :)
- At least one integration test (see
./other/misc-tests
). I don't know what would be involved, but I would like to make sure we have something in place to not break RN before we commit to supporting it. - Documentation
Once we get all of those, then we can officially announce it and commit to not breaking things (intentionally) without semver major.
…target (downshift-js#265) * Add isReactNative util function This can be used to determine if the current target is React Native. See facebook/react-native#10881 and https://github.com/facebook/react-native/blob/70c359000a2df091c3939f4c19db6024af992d43/Libraries/Core/InitializeCore.js#L194-L195 for more info. * Ensure correct onChange prop is supplied for React Native * Configure onPress item prop * Ensure navigator is defined before calling for a property * Fix typo * Wrap mouse event listener in non-ReactNative checks * Ensure call to get item node from index throws on React Native * Ensure correct event handler is passed to getButtonProps * Ensure no calls to getItemNodeFromIndex on React Native * Use a safer check for testing for React Native This fixes errors that are thrown on SSR builds * Add build script for React Native product * Use preval macro to determine if product is React Native This should help eliminate some overhead to non-React Native targets. * Add react-native directory to gitignore * Add react-native directory to distributed files This matches the same delivery technique as preact * Remove unnecesary comments * Remove React Native specific error * Update comment about isReactNative * Move isPreact preval to utils This will match the isReactNative preval as well, unifying these methods. Additionally, we can drop the Boolean cast, improving minification, if we disable Istanbul in here. * Disable coverage on React Native specific codepaths Since we don't have a test suite for React Native yet, we can't cover these paths. * Add react-native package directory This will allow for a separate react-native build using this directory * Fix bad rebase * Ensure that internal state is still set on React Native * Add separate command for building web The build command now builds for both web and native targets * Use babel over Rollup for React Native builds This will prevent us from smashing against different build targets by sharing a build directory. This also uses the "react-native" key in the package.json to tell Metro Bundler where to get the module from. * Prefer using inline prevals over exporting a constant This will allow Rollup to optimize builds for UMD, ESM and CJS, without minification. * Remove unneeded call to scrollHighlightedItemIntoView * Further tweaks for react native build setup * Use cross-env to set env vars
What: Adds support for using 🏎 in React Native. Closes #185.
Why: Prior to this, React Native could not be targeted, as there are DOM-specific codepaths. This ensures those codepaths are not followed on React Native.
How: Added the basic
downshift
implementation into a React Native app and started hammering out errors, stripping away DOM-related code.Checklist:
None of these are done yet! This is a work-in-progress. Putting this up for review as a check to make sure I'm headed in the right direction.