Skip to content

Conversation

kamilio
Copy link

@kamilio kamilio commented Sep 23, 2015

when document does not exist

While using great library react-unit to test the React components I discovered that this function is the only part that needs document global otherwise it crashes (variable not defined). This is an attempt to make it work even without document.

I've added simple test to verify basic functionality, but haven't figure out how to test my scenario when document is not present.

Thanks a lot and looking forward for the feedback...

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@kamilio
Copy link
Author

kamilio commented Sep 23, 2015

I signed the CLA already..

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

*/
function getActiveElement() /*?DOMElement*/ {
var localDocument = (typeof document === 'undefined') ? {} : document;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we go with this I would ultimately just shorten it - we know that we're going to return undefined when typeof document === 'undefined' - no point assigning to an empty object and entering a try/catch.

@zpao
Copy link
Member

zpao commented Sep 24, 2015

cc @salier, @yungsters for a sanity check before going further

@yungsters
Copy link
Contributor

Seems reasonable, but agree with @zpao.

if (typeof document === 'undefined') {
  return null;
}
// ...

@kamilio
Copy link
Author

kamilio commented Sep 25, 2015

Hello, early return makes sense, thanks. Pushed new commit into my branch. Please review.

@kamilio
Copy link
Author

kamilio commented Sep 25, 2015

Should I squash the commits or is it fine?

@yungsters
Copy link
Contributor

Squashing is ideal. :)

Soon, we will be able to automatically squash. But for now, manually squash is desired. Thanks!

…does not exist

While using great library `react-unit` to test the React components I discovered that this function is the only part that needs `document` global otherwise it crashes (variable not defined). This is an attempt to make it work even without document.
@kamilio kamilio force-pushed the getActiveElement-without-dom branch from 7adb390 to 4fc5b49 Compare September 25, 2015 08:27
@kamilio
Copy link
Author

kamilio commented Sep 25, 2015

Squashed and forced push.

Tell me more about the automatic squash, is it desired to have each PR squashed into single commit? Or is it going to be on demand, wherever sensible?

@yungsters
Copy link
Contributor

I'm in the process of getting this project automatically synchronized with our internal repository at Facebook. This is how Relay and React Native currently work.

Once that happens, each merged PR will automatically be squashed into one commit.

yungsters added a commit that referenced this pull request Sep 25, 2015
Fix getActiveElement so that it does not throw error
@yungsters yungsters merged commit 0ba6938 into facebook:master Sep 25, 2015
@kamilio
Copy link
Author

kamilio commented Sep 25, 2015

Thanks a lot, @yungsters and what is the process of releasing React. When should I expect this PR be part of it?

@zpao
Copy link
Member

zpao commented Sep 25, 2015

We'll push out a new fbjs version from here soon and then update the React dependency. 0.14 should be going out soon and I'll make sure it includes this.

@jakepusateri
Copy link

This got reverted in 03ce16e . Was that intentional? Upon upgrade to React 0.14.5, I'm getting many errors that look like this while running unit tests. These errors are not present in React 0.14.4

     ReferenceError: document is not defined
      at getActiveElement (node_modules/react/node_modules/fbjs/lib/getActiveElement.js:25:12)

@zpao
Copy link
Member

zpao commented Jan 4, 2016

No, I don't think that was intentional. @yungsters - it looks like we never pulled this commit internally after merging :(

@yungsters
Copy link
Contributor

Acckkk... I'll re-commit this.

yungsters added a commit to yungsters/fbjs that referenced this pull request Jan 5, 2016
@conartist6
Copy link

Sorry, but I am planning to to pull this out. When we add type checking to it becomes clear that the vast majority of facebook's code would not be prepared for this value to be undefined. Also this is basically an alias for writing document.activeElement, which is not something that should be written when there is no document. React itself has a lot of code which will run when document is not defined, and checks should be done there by checking the ExecutionEnvironment.canUseDOM property when appropriate. I have looked through React in an attempt to determine if any current usage of getActiveElement could possibly be called when document is undefined, but I don't see any.

If you can provide your a way that I can reproduce your issue I will ensure that it stays fixed, otherwise I will assume that in the last year React fixed the root cause of the issue.

@kamilio
Copy link
Author

kamilio commented Oct 11, 2016

Go ahead, should be fine :) Thanks for asking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants