-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[TM] Add spec for UIManager #24902
[TM] Add spec for UIManager #24902
Conversation
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow |
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 you use strict-local and update references to Object which is implicit any?
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.
@thymikee object is a desired return result according to the issue filed. In codegen, I think it has a different meaning from any.
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 tend to use {[key: string]: mixed}
instead of Object. I'm not sure what assumptions codegen is making, but I would expect it (sooner or later) to get data from Flow Type AST
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.
For now, let's stick with Object
. The internal codegen isn't very smart yet, so we can improve it over time later.
So, I think the Spec should focus on what exists in native classes. For props that are updated during runtime, you can introduce a wrapper object that provides those methods. For example:
|
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.
so yeah, let's remove the non-readonly functions from the Spec
@fkgozali I wrapped the native module that I was originally exporting from UIManager.js in a type that extends the spec, the other overrides are in native code too- so im not 100% sure we want to remove them from the spec, they get modified later and call their originals. not sure if legal. |
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.
Code analysis results:
flow
found some issues.
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.
@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @ericlewis in a0879ce. When will my fix make it into a release? | Upcoming Releases |
Summary: part of facebook#24875. Because some of the methods are rewriteable, I dropped the `+` from the signature, this doesn't feel right to me, but I am not sure if the codegen requires that. If it does, it will probably be better to extend the spec and allow those specific methods to be overriden in a UIManager.js interface. Thoughts on that fkgozali or RSNara? ## Changelog [General] [Added] - Add TM spec for UIManager Pull Request resolved: facebook#24902 Reviewed By: hramos Differential Revision: D15551356 Pulled By: fkgozali fbshipit-source-id: 076c4ce635aa7ea41e21cbd67c47ecd562fc320d
Summary
part of #24875. Because some of the methods are rewriteable, I dropped the
+
from the signature, this doesn't feel right to me, but I am not sure if the codegen requires that. If it does, it will probably be better to extend the spec and allow those specific methods to be overriden in a UIManager.js interface. Thoughts on that @fkgozali or @RSNara?Changelog
[General] [Added] - Add TM spec for UIManager
Test Plan
Flow passes.