lelandrichardson / react-primitives

Primitive React Interfaces Across Targets
MIT License
3.09k stars 108 forks source link

Use React360 for VR support #106

Closed kkemple closed 5 years ago

kkemple commented 6 years ago

React VR has moved to react-360 namespace (docs). This PR just updates name of package and that works 🎉

screen shot 2018-05-02 at 4 36 21 pm
kkemple commented 6 years ago

ugh, just saw prettier did it's thing, let me know and I will remove those changes!

kkemple commented 6 years ago

sorry for the delay on this, updated!

kkemple commented 6 years ago

there was no dependency for react-vr in package.json, i assumed that was intentional

ljharb commented 6 years ago

You’re probably right. Should the file be renamed?

kkemple commented 6 years ago

Oh, that's a great question, I think vr still makes sense but could see it being called react-360 or something, did you have a name in mind?

ljharb commented 6 years ago

i think having it match the package name makes the most sense.

ljharb commented 6 years ago

However, we could keep the original react-vr intact; then this becomes a semver-minor change instead of a major.

kkemple commented 6 years ago

Looking at https://github.com/lelandrichardson/react-primitives/blob/master/src/index.vr.js#L1

I can't see how to make the split between 360 and vr since injection script is required in index.vr.js which will load for either platform unless index.360.js is supported and we can add a new root entry point. Investigating that now.

ljharb commented 6 years ago

yes, that's what i was thinking - adding a different entry point for 360, and leaving the vr one for vr.

kkemple commented 6 years ago

so .360.js isn't supported out of the box on react-360 (unless I missed something) and it looks like a significant lift to add support. Again could be missing something but I was unable to figure out how to set the platform used for module resolving, not to mention Platform.OS would need to be overridden as well.

Essentially this is too big of a lift to tackle for this PR. So we can close this until 360 is supported or we can make this a major change.

ljharb commented 6 years ago

Ah, gotcha.

Ok, what about this: do a conditional try/catch require - ie, try 360 first, then fall back to vr?

mathieudutour commented 5 years ago

a try/catch is going to lead to warnings in a webpack compilation so I'm going to merge this and publish it as a breaking change