-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
WebXR types aren't exported in 0.141 #232
Comments
Are you saying that the old way is better than the new way, or just that it changed and change is bad? From my perspective it seems like making the WebXR types ambient is the way to go since they are similar to DOM or Node types in that they aren't imported from a package, they exist in the environment/context of the JS code. |
It's a breaking change. Doesn't matter whether it's better in a vacuum -- it's our responsibility as library authors to ensure compatibility wherever possible, and this can cause nasty silent bugs in every package that depends on |
How would you suggest going about making such a change? Surely breaking changes are inevitable and the three package itself makes breaking changes every release just like we do. What are the bugs that this change causes? Does it not just cause compile errors because the types are no longer exported from the three types? |
I wouldn't, not unless it was unavoidable. Ideally, this would be inline with changes made to three. This is not the case here, hence my inquiry.
To clarify, this would only be evident for user-facing code or users with |
In what case would the WebXR types fail to resolve and thus silently fail? |
See the top issue comment, |
Right, it's the silently failing part that I'm not understanding. Are you saying those types are in a declaration file that's never getting type-checked because |
Please go back and read through this issue. This breaks existing code. I can make a PR, but it's confusing that I have to further justify the need. At this point, the conversation should be about damage control and what our options are. If the premise of the issue, which is that this could be done in a non-breaking way, is not true then we can close this issue. |
I understand it breaks existing code. We have different philosophies of breaking changes. In my opinion, |
Then you understand, while completely avoidable, this will require major semver in every library that depends on |
It depends, types and SemVer have always had a tenuous relationship. Can you give me an example library so I understand the practical problem holistically? I feel like it really depends on the details of how the library is using the WebXR types and what kind of dependency it has on |
It does not depend in this case, a hike of a peer dependency would be a breaking change (major semver). This is not something that can be worked around in user-land via If you need an example, I first noticed this when using https://github.com/pmndrs/react-three-fiber, which augments its |
Three.js is not in control of the WebXR types. They don't ship changes to WebXR, they ship changes to their use of WebXR. Browser vendors ship changes to WebXR, thus the types need to be separate from Three.js. A browser update could change the underlying APIs, but with the old way of exporting types defined in this package, there'd be no easy way of updating just the WebXR types. Typechecking would succeed, giving no indication that the underlying Three.js implementation was broken. Additionally, @types/three exporting its own definition of WebXR types clashes with any use of @types/webxr that Three.js does not cover. In my own projects, I utilize some of the WebXRLayers features on the Oculus Quest. @types/three's WebXR types were not compatible with @types/webxr, requiring rather unsightly casts to any to get typechecking to pass. By utilizing @types/webxr as the single source of truth on WebXR types, implementing projects may use Three.js and extend into their own use of WebXR without as much trouble. IIRC, the motivation for @three/types to export WebXR types was because @types/webxr was woefully out of date. That is no longer the case. As for your concern about major dependency version bumping, pre-v1.0.0 packages should consider minor version changes to be major version changes. Three.js not excluded. If you're upgrading Three.js in your project (regardless of what @types/three does), you should be considering that a major change of your own project, too. |
You don't seem to understand the issue. Remember,
This is simply untrue. You must be confused with vendored types which were exported. This is not the case if
Again, see the above.
I can hope that everyone is on the same page on how semver and software distribution works, but that is besides the point of this issue. There were no related code changes in three r141, only |
.d.ts files are not real types. You can't really "vendor" them, in the same way that one might vendor an old DLL. They are shortcuts for the typechecker, built on a promise and a prayer. But they are not functionality. You have to trust that the @types package accurately reflects the library or API it claims to describe. If it doesn't, you won't find out until runtime. They help you make sure your code is consistent with the assumptions of those types, but they are fundamentally unsound. So a library like Three.js doesn't ship a certain version of WebXR. It has to try to work with whatever version of WebXR is on the system. By originally vendoring @types/webxr, @types/three made the erroneous claim that Three.js used a certain WebXR API version. This is an impossible statement to make, and by ending the vendoring of @types/webxr, this becomes more clear. It's not clear to me what the purpose would be of a types package that chose self-consistency over accuracy. This is a trivial break. It's not like things were rearchitected and now the new API looks nothing like the old. We've corrected a major error in our type definitions, one that has nothing to do with the state of Three.js. |
I'm not sure what this comment has to do with the above discussion. For the record, I greatly appreciate your herculean efforts in |
A pull request would be helpful here in more ways than one |
So having read the thread (thank you all for maintaining a discussion about this) I do see both sides of the argument. On the one hand, breaking changes are annoying and you can argue we should try to avoid them, the reality for a library like threejs and subsequently the types for said library, it's not exactly possible because of the approach the maintainers have taken on their own versioning scheme. Secondly, imo threejs should never have shimmed their own Which leads me to my next point, afaik no one else specifically exports a name-spaced version of a browser spec and we really should take the lead from the community on this one and not go against the flow... I do sympathise with the "fail silently" but i'm struggling to reproduce this as it blatantly gives me an error in this codesandbox – https://codesandbox.io/s/async-wave-4eusoo?file=/src/App.tsx if you can provide an example of where it breaks silently it might help us understand the problem better. I don't think anyone is asking you to "justify" your requirements, it's about understanding the problem space correctly first before plowing into a solution we think is best as not to harm the core purpose of the library which is realistically to type |
Same thoughts here, although I want to distinguish breaking changes from three-related code changes that are mirrored in
I suppose this would be more specific to the usage of import * as THREE from 'three'
export type RenderCallback = (state: RootState, delta: number, frame?: THREE.XRFrame) => void
/**
* Executes a callback before render in a shared frame loop.
* Can order effects with render priority or manually render with a positive priority.
* @see https://docs.pmnd.rs/react-three-fiber/api/hooks#useframe
*/
export function useFrame(callback: RenderCallback, renderPriority?: number): null With usage being: import { useFrame } from '@react-three/fiber'
useFrame((state, delta, frame) => {
frame.foo()
})
Personally, I've hiked peer deps to prepare for a major in https://github.com/pmndrs/react-xr and https://github.com/pmndrs/react-three-fiber (aside from the big chain behind |
As I have mentioned previously, types and SemVer have a tenuous relationship. The types packages on DefinitelyTyped do not follow SemVer. Not only that, their dependencies on other types packages allow for any version (i.e., On top of that it's worth noting that If you are convinced that this breaking change requires libraries that have an implicit optional peer dependency on If you have any suggestions for how |
Typesafety breaks in react-three-fiber if anything other than ambient types are used. How does this not warrant a major bump since r141 as its interfaces are consequently not compatible with older types? Is this something that was considered in such policy in DefinitelyTyped?
Thanks for the thorough look, although this is a bit troubling. Is there a way to declare JSX interfaces and respect semver? I'm not sure if they can be declared dynamically, and semver becomes meaningless if majors are published in lockstep with three. react-three-fiber does not necessarily care about the version of three it's using, just the pieces that combine to create a scene-graph and any surrounding interfaces (e.g.
Rather than open-ended, do you mean having clear start and end versions? What would be a non implicit way to have
FWIW, my suggestion would be to have this conversation about the need/options around a breaking change as it's being made. I don't understand what you mean in regards to policy on semver, I think you're reading too much into this issue beyond investigating a way for compat. |
It doesn't warrant a major bump if you follow TypeScript/DefinitelyTyped's philosophy that types don't need to follow SemVer. My guess is that they have this philosophy because of situations like this. Trying to follow SemVer with types is much harder than following SemVer with JS. The other reason it might not warrant a major bump is because
You would have to make a major bump each time you referenced a type that was added in a more recent version. There is no way to dynamically load types. This is one of the reasons it's harder to follow SemVer for types than JS and why I think it would be impractical/unhelpful for
Yes, a package that is trying to strictly follow SemVer should have a clear start and end version. Currently To be clear, I'm not necessarily advocating that
My struggle is that so far your main argument has been that we made a breaking change and we should not make breaking changes if it's avoidable. A direct result of this argument is that we would never, at any point be able to remove the export of the WebXR types because it would be a breaking change. I am fundamentally opposed to the concept that we would never be able to make a breaking change like this. Therefore I have no interest in discussing a backwards-compatible solution (if one is even possible) if this kind of breaking change would never be possible. If we did find a backwards-compatible solution, I would be strictly opposed to adopting it if it meant that we would never be able to remove the export of the WebXR types. Breaking changes need to be allowed from time to time and I still feel that your philosophy of breaking changes is unrealistic. |
I have only suggested investigating a way for compat around the usage of ambient WebXR types if this was an unintentional breaking change. This is best explained in #232 (comment), which I seek clarification on. My initial mention of "restore WebXR.d.ts" referred to changes since and before #223, as it is unclear whether there was a point where |
Thanks for the clarification, I was under the impression that you were against the change on the basis of it being a breaking change and not whether it was intentional or not. We effectively stopped exporting types from The only discussion about the breaking nature of the change was #223 (comment) where I didn't think the effect of the change to be too burdensome for users, especially since it meant they were getting better WebXR types which I assumed would have its own subtle breaking changes whether it was exported from I am happy to discuss whether we really want to make this breaking change if there is disagreement about that on the merits of the change itself and the changes it requires for libraries and users, but I believe it to be for the best. I have added this breaking change to the release notes. |
Gonna close this for now since we don't have any plans to change this, but we can always reopen it if people want to discuss it more. |
So, I don't want to rehash this argument, but I'm hoping for some pointers. I got bit by this breaking change trying to update the three types in |
You should make a PR to DefinitelyTyped with any missing types. Until that PR gets merged, you should be able to augment the interfaces with any missing properties (like this). |
Thank you, that solved it! |
DOM Overlay is now in @types/webxr |
three
r141:node
N/A:npm
(oryarn
) N/A:Problem description:
Related: #223. Since r141, WebXR types are no longer exported alongside three and are now ambient. This is a breaking change that will cause projects' imported WebXR types to fail silently.
Relevant code:
Suggested solution:
Re-establish webxr/WebXR.d.ts, possibly re-export in addition to supporting ambient types (IDK how many people have migrated already, but best to minimize change).
The text was updated successfully, but these errors were encountered: