-
Notifications
You must be signed in to change notification settings - Fork 312
Fix getActiveElement so that it does not throw error #59
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
Fix getActiveElement so that it does not throw error #59
Conversation
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! |
I signed the CLA already.. |
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; |
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 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.
cc @salier, @yungsters for a sanity check before going further |
Seems reasonable, but agree with @zpao.
|
Hello, early return makes sense, thanks. Pushed new commit into my branch. Please review. |
Should I squash the commits or is it fine? |
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.
7adb390
to
4fc5b49
Compare
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? |
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. |
Fix getActiveElement so that it does not throw error
Thanks a lot, @yungsters and what is the process of releasing React. When should I expect this PR be part of it? |
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. |
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
|
No, I don't think that was intentional. @yungsters - it looks like we never pulled this commit internally after merging :( |
Acckkk... I'll re-commit this. |
Summary: Brings back facebook#59.
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 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. |
Go ahead, should be fine :) Thanks for asking. |
when
document
does not existWhile using great library
react-unit
to test the React components I discovered that this function is the only part that needsdocument
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...