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

[email protected] update #31

Open
wants to merge 332 commits into
base: master
Choose a base branch
from
Open

[email protected] update #31

wants to merge 332 commits into from

Conversation

idkjs
Copy link
Contributor

@idkjs idkjs commented Dec 8, 2019

I have updated the package to [email protected]. There maybe a few places where we could remove the . notations where js objects are inlined, example in AR.re https://github.com/idkjs/reason-expo/blob/0e7a1f87eb6adbcbeb216b52667b8105179558dd/packages/reason-expo/src/AR.re#L522-L566

Not sure if the tests need to be adjusted. As of now, they all pass as is.

I have add new usage comments at the top of AuthSession.re. The old ones will have to be removed once accepted. https://github.com/idkjs/reason-expo/blob/e492bd72c98b7c4ca3bbc15a99d8da716d531359/packages/reason-expo/src/AuthSession.re#L14-L22

I would also like some feedback on AdMob.re and whether to use Js.Nullable.return or Js.Nullabe.fromOption in the AdMob.AdMobBanner module.

While these lines are compiling and passing tests, I dont trust them: https://github.com/idkjs/reason-expo/blob/e492bd72c98b7c4ca3bbc15a99d8da716d531359/packages/reason-expo/src/AdMob.re#L41-L58

Looking forward you your feedback to see how to proceed.

Peace/Love.

@idkjs
Copy link
Contributor Author

idkjs commented Mar 24, 2020

@peterpme this last commit updates the deps to the following:

"dependencies": {
    "expo": "~36.0.0",
    "react": "~16.9.0",
    "react-dom": "~16.9.0",
    "react-native": "https://github.com/expo/react-native/archive/sdk-36.0.0.tar.gz",
    "reason-expo": "^36.0.0",
    "react-native-web": "~0.11.7",
    "reason-react-native": "^0.61.1",
    "reason-react": "^0.7.0"
  },
  "devDependencies": {
    "@babel/core": "^7.0.0",
    "babel-preset-expo": "~8.0.0",
    "bs-platform": "^7.2.2",
    "expo-yarn-workspaces": "^1.2.1"
  },

In packages/test rn-cli.config.js becomes metro.config.js per expo expo-yarn-workspaces.

I moved around the types in ImageManipulator.re and Camera.re to get rid of the bucklescript warnings.

.gitAttributes updates OCaml to Reason.

I feel like there are too many deps in https://github.com/idkjs/reason-expo/blob/cf55209ba848248702446c82fabd4b2bc24bcec6/packages/reason-expo/package.json#L24-L41 based on what was there before but that is what I was left with after working through the build errors. Maybe you have some insight there.

Using sdk-36.0.0.

The demo runs on web:
Screen Shot 2020-03-24 at 5 45 06 PM

And Ios:
Simulator Screen Shot - iPhone 11 Pro Max - 2020-03-24 at 17 46 10

Not sure if that is what you all wanted to do. Thoughts?

@peterpme
Copy link
Collaborator

so far looks great! just a couple merge conflicts

@idkjs
Copy link
Contributor Author

idkjs commented Mar 25, 2020

So work with me, brother. What is the protocol for resolving merge conflicts? Is that for me to do? if so how do I see the conflict? I'm greyed out in this ui.

@peterpme
Copy link
Collaborator

Hey @idkjs, I actually just noticed that as well! After some investigation, I'm not sure why .gitattributes is a conflict because it doesn't exist.

Is it easy for you to pull down master and then merge it into your branch?

For both .gitattributes and packages/reason-expo/package.json override your new version. I looked into how Expo's new stuff works and it aligns with what you're doing.

That should make fixing the merge conflict easy!

@idkjs
Copy link
Contributor Author

idkjs commented Mar 26, 2020

It would be easy if I new how to do it. Seems like if I do that, it will override all everything I updated, no? I could just change the .gitattributes manually but not sure what the problem is with package.json. Any guidance? Thanks.

@peterpme
Copy link
Collaborator

I don't think so:

on your local fork:

git checkout master
git pull
git checkout idkjs:bs_platform_7
git merge master

it should be as that? maybe i'm missing something?

@idkjs
Copy link
Contributor Author

idkjs commented Mar 26, 2020

Ran your commands:

~/G/reason-expo ❯❯❯ git branch
* bs_platform_7
  master
~/G/reason-expo ❯❯❯ git checkout master
Already on 'master'
Your branch is up to date with 'origin/master'.
~/G/reason-expo ❯❯❯ git pull
Already up to date.
~/G/reason-expo ❯❯❯ git checkout bs_platform_7
Switched to branch 'bs_platform_7'
Your branch is up to date with 'origin/bs_platform_7'.
~/G/reason-expo ❯❯❯ git merge master
Auto-merging packages/reason-expo/src/SQLite.re
Auto-merging packages/reason-expo/package.json
CONFLICT (content): Merge conflict in packages/reason-expo/package.json
CONFLICT (modify/delete): .gitattributes deleted in master and modified in HEAD. Version HEAD of .gitattributes left in tree.
Automatic merge failed; fix conflicts and then commit the result.
~/G/reason-expo ❯❯❯                                                                                                         ✘ 1 

So how do we fix these conflicts?

.gitattributes Outdated Show resolved Hide resolved
@peterpme
Copy link
Collaborator

Hey @idkjs nice! Really close. One merge conflict in package.json. Then a few questions pertaining to the Svg.re file and .gitattributes that was deleted

@idkjs
Copy link
Contributor Author

idkjs commented Mar 27, 2020

I think we are good now. Other issues?

Thank you.

wrong binding signature
@idkjs idkjs requested a review from peterpme March 27, 2020 18:44
@zth
Copy link

zth commented Apr 10, 2020

Nice work @idkjs , looking forward to using this once it lands!

@sagarjs
Copy link

sagarjs commented May 23, 2020

any updates on this pull request?
looking forward to the merge

@peterpme
Copy link
Collaborator

Hi sorry about this delay, thank you @idkjs !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.