Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Work In Progress: Split Up Index #29

Closed
wants to merge 31 commits into from

Conversation

jamestalmage
Copy link
Contributor

@urish
Don't merge yet, I just opened so you could monitor progress.
I will ping you when I am ready for a merge

@urish
Copy link
Owner

urish commented Oct 27, 2015

thanks @jamestalmage ! I will get 0.5.0 released soon with all the features we have so far.

Are you using firebase-server in one of your projects? How did you find out about it?

@jamestalmage
Copy link
Contributor Author

I found out about it on the firebase blog.
I have not switched any projects from mockfirebase yet, but I have been clamoring for something like this for a long time.

@urish
Copy link
Owner

urish commented Oct 28, 2015

very nice to learn about that :)

0.5.0 is out!

@urish
Copy link
Owner

urish commented Nov 15, 2015

@jamestalmage are you still on it pal?

@jamestalmage
Copy link
Contributor Author

Haha,
I've been off doing other stuff, but yeah, this PR needs to be finalized.

Have you taken a look? Are you OK with where I am at so far?


module.exports = ClientConnection;

function ClientConnection(ws, server) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use ECMAScript 6 classes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am only using ECMASCRIPT 6 in the tests.

See the ignore option in test/mocha-babel.js.

I hunted around and this seemed to be the best strategy. I've been working the AVA team recently (which is pretty Babel centric testing framework), so I may have some improvements to make when I get back into this.

@urish
Copy link
Owner

urish commented Nov 17, 2015

Yup, overall looks really good - a lot of changes to go over though, I have written some comments and hope to go over it again sometime this weekend

@jamestalmage
Copy link
Contributor Author

Just ping me with questions as they come up.

@jamestalmage
Copy link
Contributor Author

Hey @urish, I'd like not to let this go much longer without a merge. I am concerned about letting it slip out of date. Have you had a chance to review?

So far I only have one note from you, and that is to not use delegates, but write it out explicitly. If that is the only change you want, I can rebase and make that change this weekend.

@urish
Copy link
Owner

urish commented Dec 18, 2015

Good call @jamestalmage , I am on it

@jamestalmage
Copy link
Contributor Author

@urish - What do you want to do with this? I don't have a lot of time to spend fixing up the conflicts.

@urish
Copy link
Owner

urish commented Mar 8, 2016

@jamestalmage I definitely understand. I think that as long as the new code doesn't break backward comparability (I think you renamed one exported method) and all tests pass, we can get going with what you already have, and then make style fixes / improvements as we go.

Thoughts ?

@jamestalmage
Copy link
Contributor Author

@urish - Unfortunately, I don't have time to rebase this. I will leave my branch up if anyone wants to take a crack at it.

I don't think it contained any breaking changes at the time.

@urish
Copy link
Owner

urish commented Apr 28, 2016

Thank you James!
On Apr 28, 2016 11:46 PM, "James Talmage" notifications@github.com wrote:

Closed #29 #29.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#29 (comment)

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.

None yet

2 participants