Skip to content
This repository has been archived by the owner on Apr 25, 2018. It is now read-only.

Better distributable #89

Closed
wants to merge 0 commits into from
Closed

Better distributable #89

wants to merge 0 commits into from

Conversation

geekytime
Copy link
Contributor

@geekytime geekytime commented Apr 27, 2016

This one sort of works from JSBin.

I had trouble getting a more complex example to work, but perhaps I am just bad at hyperscript? It seems to me that it's the right type going in there, but maybe this is related to the type-checking problems you discussed in other issues @garbles?

I can rebase and clean up before we merge, but I wanted to get it up where we can play with it.

@geekytime
Copy link
Contributor Author

I tried another example forked from a previous example and updated for the latest build.

This one has the same problems. It appears that I broke something related to how hyperscript is rendering the virtual nodes? It doesn't look like it's properly subscribing to Observable props...

@geekytime
Copy link
Contributor Author

Even this super-simple example doesn't work when we use an observable as content.

@garbles
Copy link
Owner

garbles commented Apr 28, 2016

A few things:

  • This commit fixes the problem of using many versions of RxJS
  • The examples you've provided are using RxJS Alpha 8 which is very outdated
  • This still doesn't fix the problem of how to add operators to what comes out of createEventHandler

@geekytime
Copy link
Contributor Author

The examples you've provided are using RxJS Alpha 8 which is very outdated

Duh. It works when I update to beta-7.

This still doesn't fix the problem of how to add operators to what comes out of createEventHandler

If we're using the global UMD build of Rx do we need to add the operators?

@geekytime
Copy link
Contributor Author

Yeah, everything seems to work fine on JSBin now. Here's an example with "extra" operators.

I'm still open to ideas about the name/location of the yolk.min.js file, and whether an automated integration test using the minified file seems necessary.

@geekytime
Copy link
Contributor Author

I could also add a small section to the readme that shows how to use Yolk from JSBin. It is such a useful way to submit examples of bugs, etc.

@LongLiveCHIEF
Copy link

Too bad github doesn't do what StackOverflow started doing when you include jsbin's and the like (the execute button)

@garbles
Copy link
Owner

garbles commented May 3, 2016

This LGTM with a few notes:

  1. Would be nice to have an un-minified build (could push to another PR)
  2. Would be nice if you could rebase off of master (specs should pass now)

@jadbox
Copy link
Contributor

jadbox commented May 3, 2016

LGTM

Let's see if we can get this in today as I'm currently using @geekytime older Yolk build for our production :P

@geekytime
Copy link
Contributor Author

Would be nice if you could rebase off of master (specs should pass now)

Yeah, if you want to avoid the merge commits I can rebase. Just to be clear, you want me to force-push back to this branch, right? You're okay with me rewriting history?

@garbles
Copy link
Owner

garbles commented May 3, 2016

@geekytime

@geekytime
Copy link
Contributor Author

Er. I guess I did that wrong. My git-fu is not strong, today. 😒

@geekytime
Copy link
Contributor Author

Something I did in git ticked off github, and it closed this automatically. I opened #94 instead.

I apologize for my lack of inter-fork rebasing skills.

@geekytime geekytime deleted the better-distributable branch May 3, 2016 19:58
@geekytime
Copy link
Contributor Author

Let's see if we can get this in today as I'm currently using @geekytime older Yolk build for our production :P

Sorry if my branch problems caused you any headaches. If you need a stopgap until #94 gets merged, you can use: https://rawgit.com/StateFarmIns/yolk/add-browser-distributable/dist/yolk.min.js

@LongLiveCHIEF
Copy link

I apologize for my lack of inter-fork rebasing skills.

emperor displeased

@garbles
Copy link
Owner

garbles commented May 4, 2016

👏

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

Successfully merging this pull request may close these issues.

4 participants