Update Promise type annotations #2732
Conversation
icholy
force-pushed
the
master
branch
2 times, most recently
from
bd015ae to
eebdae9
Compare
icholy
changed the title
icholy
changed the title
|
Thanks. I'll take a closer look at this soon. It looks like jsDoc added official support for Promise annotation in 3.3.0 (we are on beta8 but can probably upgrade easily), so I don't see why we shouldn't take advantage of them. One thing I did notice is that many of your annotations appear to be incorrect (at first glance). For example, loadImage always resolves to an |
|
@mramato its still a WIP, ill ping you when it's ready for review. I wasn't aware of the promise support in jsdoc, I'll check it out. |
| @@ -135,7 +135,7 @@ define([ | |||
| * Gets a promise that, when resolved, indicates that the EOP data has been loaded and is | |||
| * ready to use. | |||
| * | |||
| * @returns {Promise} The promise. | |||
| * @returns {Promise.<Object>} The promise. | |||
There was a problem hiding this comment.
Should this be EarthOrientationParameters ?
There was a problem hiding this comment.
Currently, this promise resolves to no value, i.e. undefined. We use this pattern in a few places to indicate that some process has finished, but there is no obvious result value of the process.
This particular class is marked "private", though, and is considered undocumented, so I don't think we need to worry about it for now.
In general, perhaps we should perhaps always try to return some value from all promises, even when not really necessary. Perhaps as you suggest this loading promise could resolve to the EarthOrientationParameters object, even though the caller must have already had a reference to that object in order to access the promise in the first place.
|
@mramato I think it's almost there. Take a look when you have a minute. I can't find the jsdoc promise support you mentioned. I read through this issue, but there doesn't seem to be anything implemented. |
icholy
changed the title
|
Rebased & Squashed |
|
Are there any issues with this this that I still need to address? Or is it just going to take some time to get to/review? |
|
We just need to take the time for a closer look and review. |
|
What's the plan with this PR? |
|
I need these changes to move forward with making TypeScript definition files. |
|
The main problem is determining the impact on our generated documentation. Last I checked, while this was a proposed syntax for jsDoc, it wasn't supported yet. I need to check out the latest version and see if that's changed. Even if they don't support it, we may be able to take this change (as long as the doc still compiled and doesn't drop information). |
|
Here's what it looks like Here's what it looked like before: edit: AFAIK, no promise support has been added to jsdoc yet. |
|
Thanks @icholy I'll take a final look at this tomorrow and assuming no issues, we can merge it in. Thanks again. |
|
It may have to change in the future when JSDoc gets official Promise support, but it looks like they may choose this syntax anyway. I'll make a tweak in master to change |
Update Promise type annotations
|
@icholy thanks again for this contribution. We appreciate all your pull requests. |
Here's a start for#2730