Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix inconsistency in result parameter #7444

Closed
OmarShehata opened this issue Dec 29, 2018 · 3 comments
Closed

Fix inconsistency in result parameter #7444

OmarShehata opened this issue Dec 29, 2018 · 3 comments

Comments

@OmarShehata
Copy link
Contributor

It sounds like the intention is to force users to supply a result parameter to prevent accidental bad performance (based on the discussion here #1161).

There's still a lot of functions that have an optional result parameter (were these added after that issue was resolved?) For example, all the methods below have an optional result in Cartesian3:

Similarly for all the functions here:

https://cesiumjs.org/Cesium/Build/Documentation/Transforms.html

I personally like it when it's optional because I usually just end up throwing an inline new Cesium.Cartesian3() since it's usually something that runs only once, but I think regardless of my personal preference the library should be consistent. It's very annoying to try to remember which methods required a result and which didn't.

@mramato
Copy link
Contributor

mramato commented Jan 2, 2019

  • fromXXX elements are almost always used to create new instances so they intentionally do not require a result parameter. This shouldn't change.

  • I think Transforms was just missed the first time around and it would be a hassle to update everything to enforce it (since it's a breaking change).

I think everyone would love to get rid of result parameters completely, but they are an unfortunate requirement for writing performant JS. We could probably just close this unless there are lots of other examples of non-fromXXX functions that have it as optional.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 2, 2019

Yeah, I agree. The API is this way by design.

@hpinkos hpinkos closed this as completed Jan 2, 2019
@OmarShehata
Copy link
Contributor Author

Thanks for the context. I'm fine with closing. I (or anyone else) can re-open if more functions like that are found that were missed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants