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

Feat/react-native-exports #222

Merged
merged 12 commits into from
Jan 18, 2021
Merged

Feat/react-native-exports #222

merged 12 commits into from
Jan 18, 2021

Conversation

joshuaellis
Copy link
Member

@joshuaellis joshuaellis commented Jan 8, 2021

Resolves #221
Resolves #212
Resolves #182
Resolves #19

Looked at what react-spring does and used that as a basis for this PR to fix those issues. Created a native that doesn't include html and loader and a web that includes all. index imports web

@joshuaellis
Copy link
Member Author

Last thought would be - should @react-spring/web be included as a dependency. Native users don't need it... Could be a peer? Also, I will update readme after we're happy with implementations.

@gsimone gsimone requested a review from drcmda January 9, 2021 11:43
@gsimone gsimone marked this pull request as draft January 9, 2021 11:43
@gsimone
Copy link
Member

gsimone commented Jan 9, 2021

@drcmda what do you think?

@joshuaellis I converted the PR to draft so that we don't merge it until everyone is ok 👍

I think this would break the stories, if paul is ok with the change, we need to also fix the imports ( right now they import from each module file, we should import from index to catch errors early)

@joshuaellis
Copy link
Member Author

I converted the PR to draft so that we don't merge it until everyone is ok

sure that makes sense.

Yes it would break the stories, I think I was meant to suggest that we merge this into a beta, to ensure the exports work 100%. I'm working on #223 as well and this is a draft, these could compliment well together and both could be merged into a beta.

@joshuaellis joshuaellis changed the base branch from master to beta January 12, 2021 18:09
@joshuaellis joshuaellis added the request for comments Discussion about changes to the library label Jan 12, 2021
@joshuaellis
Copy link
Member Author

I've had a bit more of a think about it and I do think it's important to put Loader & Html in the web folder, I think for anyone coming to devlop it's clear that in the event that components can only be used by a particular target (although we all want to reduce that) that we put them in said folder.

@gsimone
Copy link
Member

gsimone commented Jan 15, 2021

Do you want to do that with a breaking change or keep supporting all imports from index?

@joshuaellis
Copy link
Member Author

It wouldn't be breaking (i think, feel free to correct me) as the root: import {component} from '@react-three/drei' use's /web as it's reference:

import { Loader } from '@react-three/drei' // works
import { Loader } from '@react-three/drei/web' // works
import { Loader } from '@react-three/drei/native' // does not work

This would be the same for Html too. Mearly just pushing native users to use the specific native folder, meanwhile (what I assume is most people) can carry on as normal.

@gsimone
Copy link
Member

gsimone commented Jan 15, 2021

Looks good to me, we'll add a paragraph for Native users in the README 👍

@joshuaellis
Copy link
Member Author

Awesome, will do & i'll resolve merge conflicts too.

gsimone and others added 5 commits January 15, 2021 10:32
* feat: setup TS for stories

* fix: eslint was ignore .storybook

* feat: add TS to storybook

* refactor: move stories to TS (WIP)

* refactor: move more stories to TS (WIP)

* refactor: src changes for TS move over

I think these were TS problems that were missed.

* fix: ContactShadows story

* refactor: move more stories to TS

* refactor: add MapControls.tsx after raising issues in three

Two errors linked to three.js:
mrdoob/three.js#21058
mrdoob/three.js#21059

* fix: TS errors for passing refs & children

* refactor: add more stories (WIP)

* fix: useContextBridge TS to accept array of children

fixed using DefinitelyTyped/DefinitelyTyped#44572 (comment) because microsoft/TypeScript#14729

* refactor: convert more stories to TS

also useGLTF does not need to declare it's type

* chore: update storybook

* fix: revert useTexture

accidentally committed a WIP change

* refactor: move stories to TS (WIP)

* refactor: revert useGLTF story

* Minor type fixes

* chore: update react-three-fiber to latest

Required for Types

* refactor: move stories to TS

* fix: shaderMaterial on init returns void, not null

* refactor: remove unnecessary type in useFBX

* refactor: type Environment stronger

there was issues with useAsset not understanding it's types....

* Fixes CSB CI (#230)

Co-authored-by: Gianmarco Simone <gianmarcosimone89@gmail.com>

Co-authored-by: Josh <37798644+joshuaellis@users.noreply.github.com>
if you're using react-native you don't need it.
@vercel
Copy link

vercel bot commented Jan 15, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/drei/r0rbvr1o6
✅ Preview: https://drei-git-fork-joshuaellis-feat-react-native-exports.pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 15, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 14d5ac2:

Sandbox Source
keen-snowflake-n8ovk Configuration

@joshuaellis joshuaellis removed the request for review from drcmda January 15, 2021 17:40
@joshuaellis joshuaellis marked this pull request as ready for review January 15, 2021 17:40
@joshuaellis
Copy link
Member Author

joshuaellis commented Jan 15, 2021

Conflicts fixed, readme updated, checks are passing 🙌🏼 v2.3.0 here we come!

@gsimone
Copy link
Member

gsimone commented Jan 15, 2021

@drcmda can you give it a 👍?

@gsimone gsimone requested a review from drcmda January 15, 2021 17:44
@joshuaellis
Copy link
Member Author

@gsimone he's happy to merge into beta, left a message on discord

@joshuaellis joshuaellis merged commit 226eb68 into pmndrs:beta Jan 18, 2021
@joshuaellis joshuaellis deleted the feat/react-native-exports branch January 18, 2021 08:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
request for comments Discussion about changes to the library
Projects
None yet
2 participants