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

Changed to om.next #18

Merged
merged 4 commits into from
Oct 31, 2015
Merged

Changed to om.next #18

merged 4 commits into from
Oct 31, 2015

Conversation

dvcrn
Copy link
Contributor

@dvcrn dvcrn commented Oct 26, 2015

Maybe related: #17

I'm playing a lot with om.next lately and found that it works fairly good in natal / react native as well. We should probably wait before merging this in until next hits stable, but this code works great as reference.

screen shot 2015-10-26 at 1 07 03 pm

@dvcrn dvcrn changed the title Changed to use om.next Changed to om.next Oct 26, 2015
@dvcrn
Copy link
Contributor Author

dvcrn commented Oct 26, 2015

Looks like I messed this one up. A lot of code was still pre-loaded so it seemed to work when in reality it didn't.

To make it work, we need to abstract ReactDOM (or wait until RN is using something similar that we could just plug in). In my tests overwriting it with React seems to work ok-ish because the API is still very similar. There are some things that don't work but it will get around the initial errors.

Afterwards we'll have to get rid of gdom/isElement inside om/add-root!, but after that I ran into You cannot render into anything but a top root.

@dvcrn
Copy link
Contributor Author

dvcrn commented Oct 27, 2015

David was kind enough to push alpha10 last-minute for us to experiment with. It adds support to specify a separate render / unmount function that we can use with react native.

It still doesn't work out of the box and needs another small change inside next.cljs, but in my tests when I patched it manually I got om.next running with native.

I'm confident that with the next alpha (alpha11?) we can get it running out of the box. I'll update my PR once that's out.

@dmotz
Copy link
Owner

dmotz commented Oct 27, 2015

Excellent work, and a perfect way to get familiar with Om Next.

I've been planning on adding an option to Natal that allows you to specify which React wrapper to use for the app skeleton. Once that's done and your changes are working without having to patch Om, we can merge this in and give users the option between Om Now and Om Next (and eventually others) during the init phase.

Thanks for doing the trailblazing here.

@dmotz dmotz mentioned this pull request Oct 27, 2015
@dvcrn
Copy link
Contributor Author

dvcrn commented Oct 27, 2015

As soon as I mentioned it in the om slack David pushed alpha11 with the needed changes. With that it now works out of the box 😊

dmotz added a commit that referenced this pull request Oct 31, 2015
@dmotz dmotz merged commit 65c93dc into dmotz:master Oct 31, 2015
@dmotz
Copy link
Owner

dmotz commented Oct 31, 2015

I added the ability to switch React interface templates like this:

$ natal init TestApp --interface om-next

Om stable is still the default, but we'll probably switch to Om Next once it's ready.

Thanks again for the PR.

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

Successfully merging this pull request may close these issues.

2 participants