tastejs / todomvc-backbone-es6 Goto Github PK

This might not be something docco is very good at---maybe a post-processing step that works off of some tag (#es6) or something would be better---but it would be cool if the ES6-feature-specific comments were in a different color, or maybe italic, or something. Maybe even a third column O_O. Commenting on the program logic is very different from commenting on the syntax, and I suspect most audiences will be interested in the ES6 syntax (at least for the next few years; once ES6 is commonplace I'm sure people will just be using this as the canonical Todo app reference and ignore all the syntax comments :P)

Two of them:

"but you would normally be accepted to write "

accepted should be expected

"Note the omission of the 'function' keyword as in ES6 it is entirely optional Default attributes for the todo."

Seems to be a word or two missing here.

I might be wrong though; this could be a case where Backbone or other constraints force you to accept any falsy values.

Classes are compact and we can use an `'extend' keyword

should be

Classes are compact and we can use an extends keyword

Idiomatic ES6 code will never use var. I think all your uses can be replaced with const, and there is no need for let.

Modules use string IDs, not identifiers (module "todo-app", not module TodoApp).

In general, the module usage is a bit muddled. For example, you're using globals for Backbone, but an inline module for TodoApp. And inline modules are generally not produced by humans, but instead by build tools. (Humans produce files that use top-level import and export.)

I know it'd be nice to show off modules, but I'm not sure this is the right way to do it.

Create our global collection of Todos.

Should probably be

Create our module-local collection of Todos.

Also, var should be const.

This repo is currently in a state where it would not work under correct es6 semantics (as seen in #33). Babel is much further along in implementing the complete es6 spec. If you switch to Babel you could get much closer to the spec.

In addition to #3:

• More const, less let, and never use var in ES6.
• No need for parens for single-argument arrow functions.
• Put 'use strict' at the top (although this will be unnecessary once you move to modules).
• Use .js instead of .es6.

In addition I just want to strongly second the use of shorthand object literals recommended in #3. After the var/let usage, that was the biggest thing that stood out to me.

Would be nice to npmify the traceur and backbone.localstorage libs so we can remove the lib folder.

In your explanation of arrow functions, you say:

  // Because of how they inherit `this` from the containing scope,
  // the meaning of `this` inside of them can't be changed with `call`
  // or `apply`. 

However, in your recap, you say

  ...
  // * `this` is bound to the containing scope— change the context with `call`
  // or `apply`.

It sounds like the first sentence should read, "Because of how they inherit this from the containing scope, the meaning of this inside of them can be changed with call or apply."

Am I understanding that correctly? Let me know and I'll open a PR.

I'm curious why you have lodash & underscore folders? Could a lodash.underscore be needed, or could just the Underscore references be changed into Lo-Dash?

return this.without(...this.completed());

uses spread but does not explain it. Seems like a perfect opportunity.

get isHidden() {

Even though it's technically ES5-ish, most people probably haven't seen this and would benefit from an explanation. Might be out of scope though, especially if you do this thing in your non-Traceur TodoMVC.

I think the name of the repo and app should be "todomvc-backbone-es6" and not traceur, yes it's the compiler, but it's way more interesting and understandable with backbone and es6.

The next step I'd like to tackle is separating out app.js into ES6 modules.

We could follow the same overall file structure adopted by the official Backbone app https://github.com/tastejs/todomvc/tree/gh-pages/architecture-examples/backbone/js but just make each individual file a module, or opt for something cleaner.

Traceur has support for modules (though it's unclear how close to the latest specs) and I'm considering using https://github.com/ModuleLoader/es6-module-loader which we put together as a module loader polyfill (that is spec compliant).

Note the omission of the 'function' keyword as in ES6 it is entirely optional

This seems a bit of a strange way to talk about this. I would instead have a sentence saying something like

Notice our use of the new "method definition" syntax for declaring methods for our class; we don't need the function keyword. This works in object literals, too.

@passy would you be interested in improving the level of code commenting we have on the project at present?

I think that we already have reasonable functional comments about what the code is doing, but could document what syntax is newer (ES6) where relevant.

e.g

//
// ES6 module - a unit of source code with optional imports and exports.
//
// ES6 draft: https://people.mozilla.com/~jorendorff/es6-draft.html#sec-14.3
// Examples: http://wiki.ecmascript.org/doku.php?id=harmony:modules

Classes in ES6 require super() to be called before you can use this in a constructor (now enforced in Babel 5.x), but todo-app.js needs to do this a few times to support property values such as this.events and this.tagName. Is there a way to refactor this so the properties can be initialized after calling super() (though so far I haven't been able to think of a way this would work with Backbone out of the box)?

[UPDATE] Relevant Backbone issue: jashkenas/backbone#3560

Hi,
i'm not able to run this project in chrome(regular or canary), here the console log:

XMLHttpRequest cannot load file://localhost/Users/my-name/Downloads/traceur-todomvc-gh-pages/js/app.js. Cross origin requests are only supported for HTTP. traceur.js:18614

Failed to load file://localhost/Users/my-name/Downloads/traceur-todomvc-gh-pages/js/app.js traceur.js:18599

Uncaught NetworkError: A network error occurred.

i haven't any issue in Firefox

A thought that just crossed my mind: Generators would be perfect for reading todo items from localStorage since all of them are stored under separate keys and each access is obviously blocking.

If we replaced backbone.localStorage.js with something of our own that is generator-capable, could we override Model.sync or monkey-patch Backbone to support this?

new url is https://tastejs.github.io/todomvc-backbone-es6
or http://tastejs.com/todomvc-backbone-es6/

Arrow functions are introduced in their expression form. The end of the file includes

$(() => {
    new AppView();
    new Filters();
    Backbone.history.start();
});

which would be a good chance to talk about how you can use them for statement forms (including multiple statements).

It might be worth mentioning that the () could go away soon. In fact you could even use this to write a sentence or two about es-discuss, features still changing and under flux, community feedback helpful. But that's probably out of scope.

I've managed to get a full production build workflow going.

You can see this in my fork here - https://github.com/guybedford/traceur-todomvc

The production app is at index-built.html

I haven't created a pull request, because it uses the SystemJS project, which is still not completely launched.

The build process is the following:

1. Compile js -> js-built turning ES6 in ES5 and AMD
2. Use the r.js optimizer to compile the AMD into a single file
3. Use the traceur runtime and systemJS AMD-production module (finished yesterday!) in the production environment.

Currently the above is just in a Makefile. This can be done through any other tool as necessary.

Feedback welcome.