Skip to content
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

TS: Add missing types: is*** #18259

Merged
merged 5 commits into from
Dec 30, 2019
Merged

TS: Add missing types: is*** #18259

merged 5 commits into from
Dec 30, 2019

Conversation

yomotsu
Copy link
Contributor

@yomotsu yomotsu commented Dec 28, 2019

isXXX is missing in d.ts.
This PR is to fill them.

Screen Shot 2019-12-29 at 2 51 03 AM

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 28, 2019

Um, can we mark all is* properties as read-only?

Related #18217 .

@Mugen87 Mugen87 changed the title [d.ts] Add missing types: is*** TS: Add missing types: is*** Dec 28, 2019
@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2019

Thanks for the suggestion.
Regarding it, Which one is preferable?

  • readonly isBox3: boolean
  • isBox3: true

Seems some are already isXXX: true like
https://github.com/mrdoob/three.js/blob/dev/src/math/Vector3.d.ts#L30

But we can respect the actual value in .js file (which is true) to set boolean as readonly as you suggested.

or maybe both: readonly isBox3: true

@donmccurdy
Copy link
Collaborator

I'd suggest both: readonly isBox3: true 👍

@yomotsu
Copy link
Contributor Author

yomotsu commented Dec 29, 2019

Done it 👍

@Mugen87 Mugen87 added this to the r113 milestone Dec 29, 2019
@Mugen87 Mugen87 merged commit 3ba0553 into mrdoob:dev Dec 30, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 30, 2019

Thanks!

@yomotsu yomotsu deleted the feature/add-type-isXXX branch December 30, 2019 16:05
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Jan 3, 2020
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants