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

[CS2] Remove unnecessary utility helper functions #4526

Merged
merged 10 commits into from
Apr 25, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Apr 23, 2017

Inspired by @connec’s comment:

  • Remove polyfill for indexOf, leaving the shortcut to the native function.
  • Remove utility function for bind entirely, replacing its one usage with a call to the native bind.
  • Unrelated but I slipped it in: updated the modules tests that were waiting on class to be supported

The one outlier is the utility function for extends, which remains. Now that we compile to native class and extends, there’s only one very narrow case where this helper still gets used:

    o 'SimpleAssignable EXTENDS Expression',    -> new Extends $1, $3

which pairs with:

#### Extends

# Node to extend an object's prototype with an ancestor object.
# After `goog.inherits` from the
# [Closure Library](https://github.com/google/closure-library/blob/master/closure/goog/base.js).
exports.Extends = class Extends extends Base
  constructor: (@child, @parent) ->
    super()

  children: ['child', 'parent']

  # Hooks one constructor into another's prototype chain.
  compileToFragments: (o) ->
    new Call(new Value(new Literal utility 'extend', o), [@child, @parent]).compileToFragments o

We have zero tests covering this. I suppose the hundreds of class tests used to cover it in the 1.x codebase, but now that this only applies to straight prototypal inheritance we don’t have any tests that refer to that as opposed to classes. This is mentioned in the docs, but there’s no example, just a single half-sentence: “The extends operator can be used to create an inheritance chain between any pair of constructor functions”.

Anyway for the purposes of this PR we can just leave it as is. Later on though we should probably decide whether or not we want to keep it (might as well, I suppose) and if we do keep it, it should get some tests of its own and probably an example in the docs.

EDIT

  • Removed extends.
  • Added helper for splice just like slice.

@GeoffreyBooth GeoffreyBooth added this to the 2.0.0 milestone Apr 23, 2017
@connec
Copy link
Collaborator

connec commented Apr 24, 2017

Neat!

On extends - I think we should seriously consider removing it. It doesn't make much sense as a language feature any more, especially since it behaves differently from class extends, and there's no ES equivalent for a standalone extends.

@GeoffreyBooth
Copy link
Collaborator Author

The place I see it being used is with frameworks. Consider Backbone, with lines like class AppView extends View where View is just a vanilla function. Wouldn't we still need this old extends in cases like that?

src/nodes.coffee Outdated
@@ -3118,7 +3118,7 @@ UTILITIES =

# Correctly set up a prototype chain for inheritance, including a reference
# to the superclass for `super()` calls, and copies of any static properties.
extend: (o) -> "
extend: (o) -> """
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will make the helper function take several lines in the output JS. Is this change intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, no, I was just figuring that multiline strings should always be block-quoted as a matter of style.

@jashkenas
Copy link
Owner

Neat!

On extends - I think we should seriously consider removing it. It doesn't make much sense as a language feature any more, especially since it behaves differently from class extends, and there's no ES equivalent for a standalone extends.

I agree. I think it should be removed for CS2.

The reason for its existence was CS1's "classes via JavaScript prototypes" helpers.

@GeoffreyBooth
Copy link
Collaborator Author

@jashkenas you would be the one to confirm: does removing extends pose any issues for compatibility with Backbone? I guess not, since class AppView extends View is an ES class extending a function, which I guess ES classes can do?

@jashkenas
Copy link
Owner

Even if it did — that shouldn't be a consideration that CoffeeScript needs to worry about.

But it doesn't: http://backbonejs.org/#Model-extend

@GeoffreyBooth
Copy link
Collaborator Author

Okay, I’m convinced.

  • Removed extends.
  • Added helper for splice just like slice.

Do these shortcuts, like slice for [].slice, really make a speed difference? I guess they help in minification, though I question the value of minification nowadays with gzip compression enabled.

@GeoffreyBooth
Copy link
Collaborator Author

Regardless of whether we should remove the shortcuts, does this PR look okay?

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

Successfully merging this pull request may close these issues.

4 participants