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

rewrite hoodie.js in JavaScript #51

Closed
3 tasks done
gr2m opened this issue Apr 21, 2013 · 40 comments · Fixed by #80 or #82
Closed
3 tasks done

rewrite hoodie.js in JavaScript #51

gr2m opened this issue Apr 21, 2013 · 40 comments · Fixed by #80 or #82

Comments

@gr2m
Copy link
Member

gr2m commented Apr 21, 2013

tl;dr

  • replace Cakefile with grunt for building / testing / etc
  • rewrite hoodie.js with all core modules to JavaScript, test against existing specs
  • rewrite specs to JavaScript

This is a great opportunity to set the cornerstones of the future hoodie.js, your chance to become core contributor. The only requirement is to remain the frontend API.

Leave a comment if you're interested.


hoodie.js is currently written in CoffeeScript. The reason is, that I'm very comfortable myself to write CoffeeScript that documents itself pretty well. And at some points, when the code would become too complex, I'd rather keep it simple, readable, but therefore less performant.

I think that there can live multiple implementations of hoodie.js among each other, even for the same backend. There are other libraries that have that, like underscore.js or mustache.js.

But the main implementation of hoodie.js should be in pure JavaScript, I'd still keep the CoffeeScript implementation as learning code for the people that like it.

This is a huge chunk of work. But it's also a great opportunity to gather several developers that want to reimplement hoodie.js in pure JavaScript, and compine their experience to make it super solid. And the coolest thing: the specs are already there!

Comment if you're interested to help building it.

@mroderick
Copy link

I am interested in seeing where hoodie can go, and might be interested in helping with porting hoodie.js to JavaScript.

Have you got any thoughts on how it could be ported?

@gr2m
Copy link
Member Author

gr2m commented Apr 27, 2013

once we have a few decent JS developers that would like to take over some ownership, I'd

  • make a new branch and to the rewrite in it.
  • I'd leave the specs intact, so we can make sure not to break anything.
  • Once the rewrite is done, I'd rewrite the specs as well.
  • make a pull request to master and discuss in the pull request
  • Finally, merge the branch into master.

I'd also suggest to replace the Cakefile thing with grunt

Sounds good?

@rudasn
Copy link

rudasn commented May 2, 2013

This looks like a very interesting project and I'm willing to contribute a few hrs/week on this if someone from the core team is willing to take real ownership.

@gr2m
Copy link
Member Author

gr2m commented May 3, 2013

Welocme @rudasn

yes, I take real ownership, I'll make sure that things go smoothly and I'll make sure all your questions get answered, with high priority.

Would be great to have a few more developers. I'll also ask some JavaScript heros for advice on how to build & structure the core hoodie.js library

@Acconut
Copy link
Contributor

Acconut commented May 4, 2013

I would like to contribute. This will help me to understand hoodie better.

@Acconut
Copy link
Contributor

Acconut commented May 5, 2013

My fork does already have a basic grunt implementation. At the moment is just supports compiling and minifing but I'm working on the tests. 4ca7416

@jlank
Copy link

jlank commented May 29, 2013

I'd like to contribute to this effort too. Completely new to hoodie ATM though, so I need to get familiar with it before I dive in.

@gr2m
Copy link
Member Author

gr2m commented May 29, 2013

Find me at IRC or ping me at Twitter for any questions, happy to help whenever I can

@mattfield
Copy link

Hey @gr2m, I'm really interested in helping with the rewrite.

@Acconut
Copy link
Contributor

Acconut commented Jun 17, 2013

One problem I ran into while rewriting hoodie.js is to have .coffee and .js next to each other. Is there a good grunt build script which can handles CoffeScript and JavaScript files along each other good?

@svnlto
Copy link
Member

svnlto commented Jun 17, 2013

putting myself down for this as well @gr2m

@gr2m
Copy link
Member Author

gr2m commented Jun 18, 2013

@mroderick @rudasn @Acconut @jlank @svnlto

Thanks guys for offering support on this. I'll of course remain a core contributer myself, but besides that I'd love to invite you to become one yourself. I think the rewrite to JavaScript is a great way to get familiar with the overall product, and I'll make sure to be available enough to answer all your questions as fast as possible.

So, let's get this thing started :-)

I don't know about you, but I did not work on a modular library like Hoodie before, so I'd like to do some research on other libraries to find out how they structure the source code and how they do they build process.

So I'll start by looking into jQuery & Zepto, maybe you can suggest other libraries, or have already opinions on structure & Code?

Once we agree on the overall structure and build tools, I'll fork the project into it's own repository with own issues, so we can better collaborate on it and spilt up the work among us. I'm also a big fan of (remote) pair programming, it's a great way to get to know each other, exchange ideas and get lots of stuff done at the same time.

Looking forward to hear your thoughts on it!

@svnlto
Copy link
Member

svnlto commented Jun 18, 2013

I think the rewrite to JavaScript is a great way to get familiar with the overall product

agreed.

I don't know about you, but I did not work on a modular library like Hoodie before, so I'd like to do some research on other libraries to find out how they structure the source code and how they do they build process.

I don't think the current approach is that far off to be honest, other libs like the one's you've mentioned above are doing the exact same thing. As long as it works I don't see anything wrong with it either.

Once we agree on the overall structure and build tools.

The current structure of the project seems fine, if we could use 'grunt' instead and reevaluated testing tools we should be good I think.

I'm also a big fan of (remote) pair programming, it's a great way to get to know each other, exchange ideas and get lots of stuff done at the same time.

This sounds great.

@rudasn
Copy link

rudasn commented Jun 19, 2013

What @svnlto said.

Just throwing an idea out there... What would be your thoughts on using an ES6 shim/compiler so that we are free to use some of the most recent features of javascript like classes and/or modules?

@janl
Copy link
Member

janl commented Jun 19, 2013

@rudasn one of the reasons for the rewrite is to make hoodie.js more approachable for more people. One of the barriers we try to reduce is the need for a precompiler. I’m not saying this is a bad idea, but it should be weighed against the downsides :)

@mroderick
Copy link

What is the target compatibility? ES3? ES5?

+1 for pairing, I am in Berlin

@gr2m
Copy link
Member Author

gr2m commented Jun 19, 2013

tl;dr: I'd go with ES5.

Regarding browser compatibility: IE 10 / latest Chrome / Firefox / Safari / mobile Safari / Android browser. So that gives us ES5 per default. To support older browsers, I'd ask them to load a separate shim.js before they load hoodie.js.

I agree that ES6 looks super exciting, but it's not even spec ready as far as I know. I'd go with ES5 for now, and maybe to another rewrite when we are fancy and browser support is wide enough. I don't think the rewrite would be much work at this point, the Hoodie Core will be quite slim. Modules like hoodie.email / hoodie.publish / hoodie.share will be in their own repos and can be handled separately.

@gr2m
Copy link
Member Author

gr2m commented Jun 21, 2013

@mroderick @rudasn @Acconut @jlank

Hey guys,

@svnlto went ahead and got rid of CoffeeScript, replaced Cakefile with grunt, and introduced Test'em for cross browser testing. Thank Sven, awesome work!!

I've created a milestone to gather remaining things to do, feel free to grab one or add more:
https://github.com/hoodiehq/hoodie.js/issues?milestone=3&state=open

Also, there is still some weird JS code remaining from the CoffeeScript to JavaScript compilation, so just go ahead and tweak it in the v0.2 branch!

Let's continue farther discussion at #82

@mroderick
Copy link

@svnlto went ahead and got rid of CoffeeScript

As far as I can tell, @svnlto took the output of the CoffeeScript transpiler and is tidying that up for 0.2. Is that correct?

@svnlto
Copy link
Member

svnlto commented Jun 25, 2013

@mroderick it's basically:

  • replace Cakefile with grunt for building / testing / etc
  • rewrite hoodie.js with all core modules to JavaScript, test against existing specs
  • rewrite specs to JavaScript

but yes, you're correct.

Also, @gr2m I think we're good merging this.

@gr2m
Copy link
Member Author

gr2m commented Jun 25, 2013

Morgan, if you'd like to dive into the code, I'm happy to pair and give you an intro on the overall structure and answer all your questions

Gregor Martynus

On Tue, Jun 25, 2013 at 9:36 AM, Sven Lito notifications@github.com
wrote:

@mroderick it's basically:

  • replace Cakefile with grunt for building / testing / etc
  • rewrite hoodie.js with all core modules to JavaScript, test against existing specs
  • [x] rewrite specs to JavaScript
    but yes, you're correct.

    Reply to this email directly or view it on GitHub:
    rewrite hoodie.js in JavaScript #51 (comment)

@mroderick
Copy link

I was under the impression that the goal of the rewrite was to produce a more "JavaScript idiomatic" version of hoodie.js. To me, working from the build artefacts seems awkward and less likely to achieve the goal of making hoodie.js more approachable to more people (as you're practically writing coffeescript build artefacts by hand).

If we start from the build artefacts, we are going to have to live with them for a long time to come, and the intention of the programmer(s) won't be that clear.

Are you sure this is the path you want to take?

@gr2m
Copy link
Member Author

gr2m commented Jun 25, 2013

take one of the files, rewrite it the way you'd love it to work, an explain how it's better than the current approach. Would love to see and understand your preference

Gregor Martynus

On Tue, Jun 25, 2013 at 10:31 AM, Morgan Roderick
notifications@github.com wrote:

I was under the impression that the goal of the rewrite was to produce a more "JavaScript idiomatic" version of hoodie.js. To me, working from the build artefacts seems awkward and less likely to achieve the goal of making hoodie.js more approachable to more people (as you're practically writing coffeescript build artefacts by hand).
If we start from the build artefacts, we are going to have to live with them for a long time to come, and the intention of the programmer(s) won't be that clear.

Are you sure this is the path you want to take?

Reply to this email directly or view it on GitHub:
#51 (comment)

@svnlto
Copy link
Member

svnlto commented Jun 25, 2013

Would love to see and understand your preference

+1

@mroderick
Copy link

OK, I'll take a stab at it :-)

@gr2m
Copy link
Member Author

gr2m commented Jun 29, 2013

@mroderick why do you think you'd like to look into it? I wouldn't like to delay the CoffeeScript → JavaScript rewrite for too long

@mroderick
Copy link

Do you mean 'when'?

I've been working on it today.

On 29/06/2013, at 19.32, Gregor Martynus notifications@github.com wrote:

@mroderick why do you think you'd like to look into it? I wouldn't like to delay the CoffeeScript → JavaScript rewrite for too long


Reply to this email directly or view it on GitHub.

@gr2m
Copy link
Member Author

gr2m commented Jun 29, 2013

oops, yes, I meant when, sorry!

We just merged the v0.2 branch into master, it would be best to build on top of it. I'm curious to see what you come up with

@mroderick
Copy link

We just merged the v0.2 branch into master

Ehm, ok. Are we rushing to get something out the door, or what's going on?

@gr2m
Copy link
Member Author

gr2m commented Jun 29, 2013

The current state of the JS version was already better than the CS version. And I wanted to work on some of the open issues and didn't want to do it based on the CS version. No rush, just pragmatism

Gregor Martynus

On Sat, Jun 29, 2013 at 10:34 PM, Morgan Roderick
notifications@github.com wrote:

We just merged the v0.2 branch into master

Ehm, ok. Are we rushing to get something out the door, or what's going on?

Reply to this email directly or view it on GitHub:
#51 (comment)

@mroderick
Copy link

Well, that was what I was disagreeing with and why I am dedicating my Saturday to writing this. But, the test setup in the 0.2 branch is far from easy to work with, so I don't really see it as being very pragmatic to force it upon everyone.

@gr2m
Copy link
Member Author

gr2m commented Jun 29, 2013

don't worry about it, if you come up with a better suggestion, we'll apply it together. I'm looking forward to see what you'll come up with

@mroderick
Copy link

Am putting it into a folder structure as we speak :-)

Should be visible tomorrow

@mroderick
Copy link

I've focused on the Events constructor, as that is the base of everything else.

I've put my codified thoughts into it's own respository: https://github.com/mroderick/hoodie-51

@svnlto
Copy link
Member

svnlto commented Jun 30, 2013

@mroderick I personally don't see why this couldn't have been added to the current code, testing is fairly accessable and not far off from what you've put together with buster.js.

However, https://github.com/mroderick/hoodie-51 is a good demonstration of your approach. 👍

Would you mind pointing us to the improvements in your code over the event system that's currently in place?

@mroderick
Copy link

@mroderick I personally don't see why this couldn't have been added to the current code, testing is fairly accessable and not far off from what you've put together with buster.js.

It might be fairly accessible for you. I wasn't having much luck with it. When there were failures/errors, it spewed so many lines into the console, that not all of them were captured.

Would you mind pointing us to the improvements in your code over the event system that's currently in place?

I tried to keep as close to the original as possible, as the point of this whole exercise (issue 51) was to port hoodie.js to JavaScript and not re-implement it with an entirely new architecture.

(I also came across MicroEvent during my investigations this weekend)

In my rewrite, I've optimised for

  • making the code easier to read
  • use of builtin language features over self implemented featuers

I think that the code shows more of the intention, and is easier to read. A nice side effect is less code.

(Oh, and one actually works in my rewrite)

Ensure well defined value, instead of constantly worrying about undefined or null

By moving the resposibility of defining the property object that holds all the subscriptions to the constructor (https://github.com/mroderick/hoodie-51/blob/master/src/events.js#L17-L25), we don't have to do stuff like:

// https://github.com/hoodiehq/hoodie.js/blob/master/src/events.js#L17
calls = this.hasOwnProperty('_callbacks') && this._callbacks || (this._callbacks = {});

// https://github.com/hoodiehq/hoodie.js/blob/master/src/events.js#L53-L57
list = this.hasOwnProperty('_callbacks') && ((_ref = this._callbacks) !== null ? _ref[ev] : null);

if (!list) {
    return;
}

// https://github.com/hoodiehq/hoodie.js/blob/master/src/events.js#L84-L88
list = (_ref = this._callbacks) !== null ? _ref[ev] : null;

if (!list) {
    return this;
}

Prefer builtin methods

By relying on builtin methods, the code becomes clearer (all of them have documentation and specs) and allows for better optimisation of execution (JavaScript engines have a much easier time optimising code in their space as opposed to user space).

  • Array.map
  • Array.forEach
  • Array.indexOf
  • Object.create (as inheritance method, instead of utils/deepExtend)

Size

Stats porn!

Original hoodie-51
Lines
(ohcount)
63 53
Minified
(uglify)
1332 1199
Min + gzip 537 490

@gr2m
Copy link
Member Author

gr2m commented Jul 1, 2013

I agree that your code is better to read, and that pretty much all files, especially the tests need some time to cleanup after the remained CoffeeScript → JavaScript. I'd also tend to use ES5 features and let people use shims if they need to support older browsers. Code readability is a high priority for us so your contribution to help on that is much appreciated! Just send pull requests, maybe open up issues beforehand so we don't conflict with our work, that'd be great. Cheers!

@jtenner
Copy link

jtenner commented Oct 25, 2013

ES5 Arrays tend to be slower using the Array.prototype.forEach methods. They MAY be optimized later, but for loops are just as optimizable and are clear even to beginner programmers.

Unless the function is already predefined and used somewhere else, AND smaller code size is important...

It's only my two cents. My guess is this: if they are reading the library and want to learn more, they know what a simple for loop does.

for(var i = 0, _len = arry.length; i < _len; i++){
   callFunc(arry[i]);
}

Don't get me wrong, I still like the idea of using the Array.prototype,forEach method when performance doesn't matter too much.

@mroderick
Copy link

Don't get me wrong, I still like the idea of using the Array.prototype,forEach method when performance doesn't matter too much.

This would be one of those cases

@jtenner
Copy link

jtenner commented Oct 25, 2013

Noted! It still works very quickly.

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