-
Notifications
You must be signed in to change notification settings - Fork 77
Added UMD wrapper #147
base: master
Are you sure you want to change the base?
Added UMD wrapper #147
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
hmm looks like I didn't run the linter. Hang on. |
CLAs look good, thanks! |
Thanks for sending in the PR and signing the CLA! Give me a day or two to do a quick review but it looks really good! As for tests, please send those in too when you have some time. |
Hey thanks for the all your work on the lib. I love the Firebase and plan on using it on my current project so I needed the AMD support anyway. I will get to the tests. They will take some thought because I will need to set up some environment to get them to run. For example you, thoughtfully, have Backbone in your Bower and I will need an additional copy in the Node mods so I can test for the existence of Backbone.Firebase from the runner. So weird messy stuff like that. That said I did test all three configurations by hand. |
@ChetHarrison were you able to get tests running for this? |
Moshe, Sorry I got distracted and it kinda fell off my plate. I’ll try and take a look at it again. Thanks Chet On May 10, 2016, at 11:16 PM, Moshe Brevda notifications@github.com wrote: @ChetHarrison https://github.com/ChetHarrison were you able to get tests running for this? — |
It would be a huge help @ChetHarrison! |
@mbrevda the quick fix is to grab my PR https://github.com/ChetHarrison/backbonefire. There is a lot of environment I need to think about in creating the common and amd tests that I am not a pro at but I think I can figure out. So If you are ok with that version I would get going with that while I address this. |
actually now that I think about it this shouldn't be to hard |
I would have, but your branch is a bit out of date. Also, it would be good On Fri, May 13, 2016, 6:56 AM Chet Harrison notifications@github.com
|
yes I guess I did do it a while ago. They did want to add the PR but want the tests. I am working on them now. On May 12, 2016, at 10:33 PM, Moshe Brevda notifications@github.com wrote: I would have, but your branch is a bit out of date. Also, it would be good On Fri, May 13, 2016, 6:56 AM Chet Harrison notifications@github.com
|
1 down 2 to go On May 12, 2016, at 10:33 PM, Moshe Brevda notifications@github.com wrote: I would have, but your branch is a bit out of date. Also, it would be good On Fri, May 13, 2016, 6:56 AM Chet Harrison notifications@github.com
|
Ok I have some passing mocks but it's kinda cheating. I should pull some fixtures from actual module paradigms so we know if future changes to those paradigms break the tests. But much of this mod wars BS is going to sunset, thank what ever gods you like. So I think this may be a test suite. It describes the conditions that are required to be present in order to branch to the appropriate module solution. Will PR soon. |
done |
Thanks a ton @ChetHarrison! Would you want to merge the tests to the same branch as the PR? |
@mbrevda I pushed my edits and tests to umd branch and PR'd |
but it looks like Travis is cool with it |
This PM looks approved, when is it going to be merged? |
I added the UMD wrapper so I could use backbonefire with my AMD project. It's on the "umd" branch. I tested by hand with RequireJS, CommonJS and Global implementations. I will write some tests for you next week but I thought I would check this in before the weekend hit. Happy Halloween. 😈