fix access error on document for react native #451

samuelfullerthomas · 2018-06-01T08:42:51Z

What:
Solution for this issue: #334
Handles the other cases where document is undefined

Why:
Error getting thrown for React Native due to document being undefined

I think I could use some help in thinking of the best way to write a test for this.

Checklist:

Tests
Ready to be merged
Added myself to contributors table

samuelfullerthomas · 2018-06-01T10:08:26Z

The underlying issue is this line: https://github.com/paypal/downshift/blob/master/src/downshift.js#L95, and then the fact that, from there, there aren't checks to see if the environment is just an empty object or not.

kentcdodds

Thanks! Could you fixup a few things before we review the rest?

kentcdodds · 2018-06-01T15:36:18Z

src/downshift.js

@@ -658,12 +663,10 @@ class Downshift extends Component {
    this.inputId = firstDefined(this.inputId, rest.id, `${this.id}-input`)
    let onChangeKey
    /* istanbul ignore next (preact) */
-    if (preval`module.exports = process.env.BUILD_PREACT === 'true'`) {
+    if (this.props.isPreact) {


I'm sorry, but doing this will prevent these statements from being eliminated during uglification resulting in a bigger bundle. Please restore all of these to what they were before.

kentcdodds · 2018-06-01T15:36:50Z

README.md

@@ -59,52 +59,52 @@ harder to contribute to.

 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->

-* [Installation](#installation)


I think your editor may have messed this stuff up. If you could run npx kcd-scripts format before you commit that will ensure that everything's formatted properly. Thanks!

It most assuredly did 😬

samuelfullerthomas · 2018-06-01T16:18:33Z

addressed, I think

kentcdodds

This looks good to me! Thanks :)

kentcdodds · 2018-06-01T16:30:58Z

Ok, yeah, so I'm not sure what the preval is not defined issue is all about in the build. Are you getting that locally? Try running npm run validate to see.

As for code coverage, we probably should set up some way to test the native build, but unfortunately that would take a fair amount of work. So if you can't cover some code due to react native, then add some well-placed /* istanbul ignore next */ comments. You can see the coverage report by opening coverage/lcov-report/index.html in your browser. Good luck!

samuelfullerthomas · 2018-06-02T10:38:59Z

@kentcdodds passing tests now!

kentcdodds

Just one thing. Everything else looks swell 👍

kentcdodds · 2018-06-02T13:29:06Z

other/react-native/__tests__/__snapshots__/render-tests.js.snap

@@ -9,9 +9,7 @@ exports[`renders with React Native components 1`] = `
    aria-expanded={false}
    autoComplete="off"
    id="downshift-0-input"
-    onBlur={[Function]}


Hmmm... It feels like this would be a regression.

Shouldn't this snapshot (the react native snapshot) reflect that the code in the react native build doesn't handle those props / that they won't be there? I could pass through noops but I don't think that'd be right.

Right, but before your changes, the input had these event handlers for react-native and after your chances it doesn't. That's the regression I think.

Agreed. We should ensure that the onBlur handler is still passed here.

eliperkins · 2018-09-05T13:51:24Z

src/downshift.js

    } else if (
      preval`module.exports = process.env.BUILD_REACT_NATIVE === 'true'`
    ) {
-      onChangeKey = 'onChangeText'
+      eventHandlers = {


What's the rationale behind removing onBlur from the event handlers that we use with React Native?

mistake on my part

eliperkins · 2018-09-05T13:51:45Z

other/react-native/__tests__/__snapshots__/render-tests.js.snap

@@ -9,9 +9,7 @@ exports[`renders with React Native components 1`] = `
    aria-expanded={false}
    autoComplete="off"
    id="downshift-0-input"
-    onBlur={[Function]}


Agreed. We should ensure that the onBlur handler is still passed here.

eliperkins · 2018-09-05T13:52:04Z

other/react-native/__tests__/__snapshots__/render-tests.js.snap

    onChangeText={[Function]}
-    onKeyDown={[Function]}


I'm fine with removing this one, as onKeyDown is unhandled on React Native.

samuelfullerthomas · 2018-09-05T15:20:05Z

Thanks for the review @eliperkins I think I forgot about this PR - I'll make the changes tonight. Apologies!

- remove error throwing focus call in react native - remove unnessary event handler

samuelfullerthomas · 2018-09-05T16:35:21Z

Hey @eliperkins and @kentcdodds - I've updated this PR to work with the master - it's ready to review again.

eliperkins

Nice! This ended up being a fairly trivial amount of code changed! 👏

kentcdodds · 2018-09-05T16:58:10Z

Thank you so much!

kentcdodds · 2018-09-05T17:08:12Z

Hmmmmm... It appears that the react native snapshots are failing. Care to give this a look?

https://travis-ci.org/paypal/downshift/builds/424888379

We can't release this until the build is working.

samuelfullerthomas · 2018-09-05T18:07:28Z

Hey @kentcdodds I've updated the snapshot - npm run test:build works for me locally now

kentcdodds · 2018-09-06T01:22:06Z

🎉 This PR is included in version 2.2.2 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

)   **What**: Solution for this issue: downshift-js#334 Handles the other cases where `document` is undefined  **Why**: Error getting thrown for React Native due to `document` being undefined I think I could use some help in thinking of the best way to write a test for this.  **Checklist**:   * [x] Tests * [x] Ready to be merged  * [x] Added myself to contributors table

kentcdodds requested changes Jun 1, 2018

View reviewed changes

samuelfullerthomas force-pushed the check-for-rn branch 2 times, most recently from 93c580e to d1f2ecb Compare June 1, 2018 16:15

kentcdodds previously approved these changes Jun 1, 2018

View reviewed changes

samuelfullerthomas dismissed kentcdodds’s stale review via 3302393 June 2, 2018 09:35

samuelfullerthomas force-pushed the check-for-rn branch 2 times, most recently from 872b711 to 1f58f8f Compare June 2, 2018 10:19

kentcdodds reviewed Jun 2, 2018

View reviewed changes

slavikdenis mentioned this pull request Sep 5, 2018

[react-native] Button props - Cannot read property activeElement of undefined #334

Closed

eliperkins suggested changes Sep 5, 2018

View reviewed changes

samuelfullerthomas added 2 commits September 5, 2018 17:30

improve react native

9564f99

- remove error throwing focus call in react native - remove unnessary event handler

add samuelfullerthomas to contributors list

7bc8022

samuelfullerthomas force-pushed the check-for-rn branch from 1f58f8f to 7bc8022 Compare September 5, 2018 16:33

eliperkins approved these changes Sep 5, 2018

View reviewed changes

kentcdodds merged commit d8d5722 into downshift-js:master Sep 5, 2018

samuelfullerthomas mentioned this pull request Sep 5, 2018

Update React Native snapshot #573

Merged

yowainwright mentioned this pull request Mar 6, 2020

[Snyk] Upgrade downshift from 2.2.2 to 2.2.3 yowainwright/Intermediate-React-Workshop#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix access error on document for react native #451

fix access error on document for react native #451

samuelfullerthomas commented Jun 1, 2018 •

edited

Loading

samuelfullerthomas commented Jun 1, 2018

kentcdodds left a comment

kentcdodds Jun 1, 2018

samuelfullerthomas Jun 1, 2018

kentcdodds Jun 1, 2018

samuelfullerthomas Jun 1, 2018

samuelfullerthomas commented Jun 1, 2018

kentcdodds left a comment

kentcdodds commented Jun 1, 2018

samuelfullerthomas commented Jun 2, 2018

kentcdodds left a comment

kentcdodds Jun 2, 2018

samuelfullerthomas Jun 3, 2018

kentcdodds Jun 3, 2018

eliperkins Sep 5, 2018

eliperkins Sep 5, 2018

samuelfullerthomas Sep 5, 2018

eliperkins Sep 5, 2018

eliperkins Sep 5, 2018

samuelfullerthomas commented Sep 5, 2018

samuelfullerthomas commented Sep 5, 2018

eliperkins left a comment

kentcdodds commented Sep 5, 2018

kentcdodds commented Sep 5, 2018

samuelfullerthomas commented Sep 5, 2018

kentcdodds commented Sep 6, 2018

		@@ -59,52 +59,52 @@ harder to contribute to.

		<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->

		* [Installation](#installation)

fix access error on document for react native #451

fix access error on document for react native #451

Conversation

samuelfullerthomas commented Jun 1, 2018 • edited Loading

samuelfullerthomas commented Jun 1, 2018

kentcdodds 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

samuelfullerthomas commented Jun 1, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Jun 1, 2018

samuelfullerthomas commented Jun 2, 2018

kentcdodds 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelfullerthomas commented Sep 5, 2018

samuelfullerthomas commented Sep 5, 2018

eliperkins left a comment

Choose a reason for hiding this comment

kentcdodds commented Sep 5, 2018

kentcdodds commented Sep 5, 2018

samuelfullerthomas commented Sep 5, 2018

kentcdodds commented Sep 6, 2018

samuelfullerthomas commented Jun 1, 2018 •

edited

Loading