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

ES5 bind #1408

Closed
wants to merge 3 commits into from
Closed

ES5 bind #1408

wants to merge 3 commits into from

Conversation

michaelficarra
Copy link
Collaborator

bind helper now uses native Function::bind when it is available and our custom version, supplied for environments without Function::bind, is now ES5 compliant with respect to construction (new) of bound functions (=>)


You may remember that this was on master until @jashkenas decided to take it off without a good alternative for the errant test case and broken bind function.

I don't think we should have bound function literals

=> ...

if they don't behave exactly as

(-> ...).bind(this)

would in an environment with ES5's Function::bind.

our custom version, supplied for environments without Function::bind,
is now ES5 compliant with respect to construction (`new`) of bound
functions (`=>`)
@jashkenas
Copy link
Owner

Thanks for putting this on a branch.

@michaelficarra
Copy link
Collaborator Author

Just doing as you suggested. Thanks for not immediately dismissing it if you didn't agree with it before. Also, did you notice that JS file diffs are now treated as binary diffs are in other projects? I remember you asking for this a long time ago on a blog post when they started doing it for some mobile application binary. Looks like they finally did it.

@TrevorBurnham
Copy link
Collaborator

Wow, that's really cool about GitHub suppressing the JS diffs.

As to the main topic here: The tradeoff is more standard behavior at the cost of a larger __bind helper (69 bytes minified vs. 254 bytes, under Google Closure Compiler), right? That sounds worth it to me. People who are trying to squeeze bytes would use self = this over => anyway. Or are there concerns about the proposed behavior?

@jashkenas
Copy link
Owner

There are concerns about the proposed behavior. I want to have a chance to give it a thorough look.

@josh
Copy link
Contributor

josh commented Jun 1, 2011

@michaelficarra Had to do a bunch of crazy sniffing stuff to detect Coffee generated JS

@jashkenas
Copy link
Owner

@josh What did you end up using for your sniffing implementation?

@josh
Copy link
Contributor

josh commented Jun 1, 2011

https://gist.github.com/1002993 - all the language detection code should be open sourced soon.

(Sorry for derailing the topic on this pull)

@jashkenas
Copy link
Owner

Fascinating -- I bet that code gets you close to a 100% detection rate, for any nontrivial committed files. Nicely done.

@ghost ghost assigned jashkenas Jun 27, 2011
@michaelficarra
Copy link
Collaborator Author

@jashkenas: ping

@jashkenas
Copy link
Owner

Thanks for the ping, but I'm afraid I won't get a chance to take a look at this too soon -- imminent deadlines.

our custom version, supplied for environments without Function::bind,
is now ES5 compliant with respect to construction (`new`) of bound
functions (`=>`)
…5_bind

Conflicts:
	lib/coffee-script/nodes.js
	src/nodes.coffee
@michaelficarra
Copy link
Collaborator Author

Great, something I did made other commits appear as part of this pull request. Anyway, the real comparison is here: master...es5_bind

@jashkenas: ping?

@tpetry
Copy link

tpetry commented Sep 19, 2011

Wouldn't it be possible to add an es5 compiler flag to CS? So the compiler could use the NATIVE bind function without needing to declare any wrapper function. Whenever you activate this flag and you'll deploy the compiled js you have to be sure that the target js runtime has es5 support or you should an es5-shim.

Ths would make the compiled code much more readable especially when compiling for nodejs. Maybe many more compiled statements could make use of the es5 features.

@michaelficarra
Copy link
Collaborator Author

@tpetry: CoffeeScript has only one compilation target: lowest-common-denominator ES3. It would be nice to be able to have multiple compilation targets, but the code generation is so deeply mixed with the AST that it'd be impossible even if we wanted to do it. I'm planning to do it the right way, but it'll be a while before that's available.

@jashkenas
Copy link
Owner

And for the record -- as discussed in other issues I can't recall at the moment -- targeting lowest-common-denominator JavaScript is very much by design. Separating the code gen from the AST may well be a worthwhile refactor, but we would still want to only ever compile to JavaScript that runs everywhere.

@tpetry
Copy link

tpetry commented Sep 19, 2011

@michaelficarra: Isolating the code generation and AST sounds great.

@jashkenas: ok. Only thought it would be a great while one of the big goals of CS is to generate readable JS, and the actual ___bind() approach is much more unintuitive than Function.bind() introduced with ES5

@showell
Copy link

showell commented Sep 19, 2011

It's pretty easy to extract an AST from CS internals, but I agree that it's hard to reuse the Code Generation code.

FWIW, see this program, which traverses the Node objects from CS and writes out a JSON representation of the AST that other programs can use without mucking around in CS internals:

https://github.com/showell/CoffeeNodesToJson

I like that LCD JS is the primary target of CS, but I also like the idea of opening up the AST for folks with different targets in mind. CS could eventually compile to JVM bytecode, for example, which would allow Ruby programmers to write more CS (via JRuby and friends). Also, Dart/Dash.

@michaelficarra
Copy link
Collaborator Author

Note to self: es-shims/es5-shim#108, es-shims/es5-shim#109

@vendethiel
Copy link
Collaborator

closing for #1363 (for now)

@vendethiel vendethiel closed this May 3, 2013
@GeoffreyBooth GeoffreyBooth deleted the es5_bind branch May 2, 2017 02:51
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

7 participants