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

use full ClassName.prototype.memberName #853

Merged
merged 1 commit into from May 2, 2016

Conversation

Projects
None yet
4 participants
@rhendric
Collaborator

rhendric commented Feb 17, 2016

... when assigning members to prototypes. As of this writing, Google's
Closure Compiler can only perform some advanced analysis and
optimizations if this form is used instead of the runtime-equivalent
prototype var.

(See for yourself! Compare the optimizations that are applied to the output of current lsc with those applied to the output of the patch. The unused method gets removed in the latter.)

test/oo.ls Outdated
A.prototype.one A.prototype.two
B.prototype.three B.prototype.four
]>
ok compiled.indexOf(..) >= 0 "missing #{..}"

This comment has been minimized.

@vendethiel

vendethiel Mar 15, 2016

Contributor

should be in test/compilation.ls

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Mar 15, 2016

👍?

@vendethiel

This comment has been minimized.

Contributor

vendethiel commented Mar 15, 2016

The rationale looks a bit.. Slim to me. Do you have a bytesize comparison of the full compiler with and without this PR?

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Mar 16, 2016

The LS compiler's bytesize increases by 0.6%, from 224612 to 226009 (this is with regenerating browser/livescript.js before and after the last commit, and sending it through the Closure compiler with advanced optimizations). But I wouldn't expect the LS compiler, being a self-contained project, to benefit from this change. A real-world comparison should involve compiling a library written in LS that exports a lot of classes, and a second package that uses only some of the functionality of that library, and minifying both together with Closure. (I tried searching for a public example of this that I could compare for you, but it's a tricky thing to search for and I've come up empty so far.)

Here is a quick survey of what some other class-supporting transpiled languages do:

Language ClassName.prototype.memberName prototype var something else
CoCo ☑️
CoffeeScript ☑️
ES6 (via Traceur or Babel) ☑️
JSweet ☑️
TypeScript ☑️

If there were more code generators using the prototype var pattern, I'd say this is on Closure to become smarter. But given that the two giants in the room, CoffeeScript and TypeScript, do it the long but easily-optimized way, I think it makes sense for LiveScript to follow suit and enjoy the same optimizations that they do.

@naturalethic

This comment has been minimized.

naturalethic commented Mar 16, 2016

I use zero classes in my projects. Classes are anathema to functional programming.

@rhendric

This comment has been minimized.

Collaborator

rhendric commented Mar 16, 2016

Good news then, @naturalethic: this decision will have zero impact on your projects!

use full ClassName.prototype.memberName
... when assigning members to prototypes. As of this writing, Google's
Closure Compiler can only perform some advanced analysis and
optimizations if this form is used instead of the runtime-equivalent
prototype var.
@vendethiel

This comment has been minimized.

Contributor

vendethiel commented May 2, 2016

@gkz does that seem like a worthwhile change?

@rhendric there should be no breaking change with this, correct? Only code size, that's probably nullified via gzipping?

@rhendric

This comment has been minimized.

Collaborator

rhendric commented May 2, 2016

Yup, no breaking change that I can think of.

@vendethiel

This comment has been minimized.

Contributor

vendethiel commented May 2, 2016

Okay. I think I'll just wait for @gkz's approval, then, I think.

@gkz gkz merged commit 737df48 into gkz:master May 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment