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

Rewrite repo in typescript #72

Merged
merged 17 commits into from
Jan 31, 2022
Merged

Rewrite repo in typescript #72

merged 17 commits into from
Jan 31, 2022

Conversation

robertjcolley
Copy link
Collaborator

@robertjcolley robertjcolley commented Dec 22, 2021

TODO

  • Export all types from index.ts
  • Fix component imports
  • Rework animation validation in TypeScript
  • Remove prop-types dependency
  • Remove react-create-class dependency
  • Create an example in Bobby's Starter App for each of the documented components
    • Viro360Image
    • Viro360Video
    • Viro3DObject
    • ViroAnimatedImage
    • ViroAmbientLight
    • ViroAnimatedComponent
    • ViroAnimations
    • ViroARImageMarker
    • ViroARObjectMarker
    • ViroARTrackingTargets
    • ViroARPlane
    • ViroARPlaneSelector
    • ViroARScene
    • ViroARSceneNavigator
    • ViroBox
    • ViroButton
    • ViroCamera
    • ViroConstants
    • ViroController
    • ViroDirectionalLight
    • ViroFlexView
    • ViroGeometry
    • ViroLightingEnvironment
    • ViroImage
    • ViroMaterials
    • ViroMaterialVideo
    • ViroNode
    • ViroOmniLight
    • ViroOrbitCamera
    • ViroParticleEmitter
    • ViroPolygon
    • ViroPolyline
    • ViroPortal
    • ViroPortalScene
    • ViroQuad
    • ViroScene
    • ViroSceneNavigator
    • ViroSkyBox
    • ViroSound
    • ViroSoundField
    • ViroSpatialSound
    • ViroSphere
    • ViroSpinner
    • ViroSpotLight
    • ViroText
    • ViroVideo
    • ViroVRSceneNavigator
    • Viro3DSceneNavigator
    • Styles
    • PolarToCartesian
    • IsARSupportedOnDevice

Possible TODO Items

  • Create a Scene superclass
  • Create a Navigator superclass
  • Determine the types of all the events (when an event fires, what is passed?)

Lingering Questions

The following list is things which I was not sure about when I came across them, or were not intuitive types for me to figure out.

Status Component Item
Viro3DObject What is morphTargets? What does it do?
Viro3DSceneNavigator viroAppProps?: any; // TODO: what is the type of this?
Viro3DSceneNavigator exitViro: this.exitViro, TODO: this was unused
Viro3DSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
Viro3DSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
Viro3DSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroButton // TODO: rather than manually expanding/setting props, we should do {...this.props} which will save us time when adding new properties
ViroButton // TODO: This was different than the other onHover callbacks
ViroScene // TODO: types for closest
ViroSceneNavigator viroAppProps?: any; // TODO: what is the type of this?
ViroSceneNavigator exitViro: this.exitViro, TODO: this was unused
ViroSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroSound soundSrc = { name: soundSrc }
ViroSoundField soundSrc = { name: soundSrc }
ViroSpatialSound soundSrc = { name: soundSrc }
ViroVRSceneNavigator viroAppProps?: any; // TODO: what is the type of this?
ViroVRSceneNavigator exitViro: this.exitViro, TODO: this was unused
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroARScene anchor functions are in a different format than everywhere else
ViroARSceneNavigator viroAppProps?: any; // TODO: what is the type of this?
ViroARSceneNavigator exitViro: this.exitViro, TODO: this was unused
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroVRSceneNavigator @todo: use Typescript function overloading rather than this inaccurate solution
ViroARSceneNavigator Error codes types for screen recording through viro

bilewinters and others added 9 commits March 3, 2021 00:07
…hen using the git repo as a target on the typescript branch, attempting to work out why.
In order to enhance the developer experience for both the ViroCommunity
maintainers and also the end developer, I've converted the repo to
typescript.

There were many instances where common event handlers were copy/pasted
across Viro Components. This makes the repo hard to maintain.

The end user can also get types, commends, and docs while developing
using an IDE. This also will help the end user avoid usage mistakes,
which in turn should reduce the load on the maintainers.
@robertjcolley robertjcolley self-assigned this Dec 22, 2021
@robertjcolley robertjcolley added the enhancement New feature or request label Dec 22, 2021
NS-BOBBY-C added 5 commits December 22, 2021 13:20
@robertjcolley robertjcolley marked this pull request as ready for review December 30, 2021 16:42
@robertjcolley
Copy link
Collaborator Author

robertjcolley commented Dec 30, 2021

@doranteseduardo I've considered this complete. I've tested everything in my starter kit, but I think it's ready to go. The issues I've found (#74 and #75) were replicated in my typescript repo and in the latest released version of viro.

There might be more documentation or scripts I need to update - essentially, tsc needs to be run to generate the dist folder before the repo can be uploaded to NPM

@robertjcolley
Copy link
Collaborator Author

robertjcolley commented Jan 21, 2022

NS-BOBBY-C added 3 commits January 21, 2022 08:20
Without the Resources folder being copied by tsc, this would not
resolve the dependencies correctly, since there are a few default
placeholder images.
@robertjcolley
Copy link
Collaborator Author

ViroVideo was broken on the ts branch since main hadn't been merged into it 😅 Merging in main and adding the resources folder to dist fixed the issue!

Copy link
Member

@doranteseduardo doranteseduardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@doranteseduardo doranteseduardo merged commit db2afa5 into main Jan 31, 2022
@robertjcolley robertjcolley mentioned this pull request Jan 31, 2022
@robertjcolley robertjcolley added this to the v2.23.0 milestone Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants