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

Translate most of Annotator to JavaScript #447

Merged
merged 34 commits into from Oct 7, 2014
Merged

Translate most of Annotator to JavaScript #447

merged 34 commits into from Oct 7, 2014

Conversation

nickstenning
Copy link
Member

As discussed repeatedly on the mailing lists and in person, the general consensus has been that a pure-JS Annotator would be a Good Thing. Pros and cons of the switch:

CoffeeScript JavaScript
Terser code Better static analysis/code quality tools
Less noisy syntax Better documentation tools
Better code style checking tools
Much bigger community
Simplifies build/dev tools
Simplifies packaging

None of the reasons I originally switched to CoffeeScript (lack of decent automated JS code quality tools, difficulty of writing cross-browser JavaScript) remain valid. With that in mind, I kind of accidentally translated most of Annotator to JavaScript. It wasn't an awful lot of work.

My intent is that we merge this large chunk, and then progressively translate the rest of the code (including the tests) to JavaScript as we go along. Do shout if you have any objections.

$ = Util.$;

// isEmpty returns a boolean indicating whether the passed object is empty
var isEmpty = function isEmpty(obj) {
Copy link
Member

Choose a reason for hiding this comment

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

Why var isEmpty = instead of just the named function definition? It seems like this whole file has this pattern.

@tilgovi
Copy link
Member

tilgovi commented Oct 6, 2014

Cleaned up var Foo = function Foo to make them simply function Foo and, more importantly, var Foo = function () to function Foo, without which the functions have no names and so are poorly labeled in stack traces. If there was some reason I'm not aware of these should be changed back, you can remove those last too commits.

@tilgovi
Copy link
Member

tilgovi commented Oct 6, 2014

👍 on all this

@nickstenning
Copy link
Member Author

The var foo = function foo() { ... }; pattern has two possible positives -- it forces people to define functions before they're used, as while the name is hoisted, the function value is not (unlike the form function foo() { ... }). Less importantly, it also (as you discovered) makes for more consistent closing forms:

        };
    };
};

rather than

        };
    };
}

But hey --- I'm not really bothered. I might rebase the changes inline, though, if you prefer it this way.

@nickstenning
Copy link
Member Author

Either way, agree with you that var foo = function () { ... } is not good. That's an oversight on my part.

@nickstenning
Copy link
Member Author

@tilgovi Rebased those changes inline into the translations.

@nickstenning
Copy link
Member Author

I'm pushing the big green button.

nickstenning added a commit that referenced this pull request Oct 7, 2014
Translate most of Annotator to JavaScript
@nickstenning nickstenning merged commit 336fce4 into master Oct 7, 2014
@aron
Copy link
Contributor

aron commented Oct 7, 2014

⭐ very happy to see this :)

@jamiefolsom
Copy link
Member

+1!

On Tuesday, October 7, 2014, Aron Carroll notifications@github.com wrote:

[image: ⭐] very happy to see this :)


Reply to this email directly or view it on GitHub
#447 (comment)
.


I would have sent you a shorter email but I ran out of time. (apologies to
Blaise Pascal)

@tilgovi
Copy link
Member

tilgovi commented Oct 7, 2014

🎺 🎉 🎆

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

Successfully merging this pull request may close these issues.

None yet

4 participants