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

Replace __proto__ references with inherits module #1753

Merged
merged 2 commits into from
Jul 5, 2015

Conversation

ndhoule
Copy link
Contributor

@ndhoule ndhoule commented Jun 17, 2015

cc @boneskull @jbnicolai

Resolves #1728.

Depends on merging #1727 because it pulls in an npm dependency; uses style from #1750

@boneskull
Copy link
Member

I think I'd rather go with LoDash here, since there's going to be a ton of other shims we can replace it with. @mochajs/mocha thoughts?

@ndhoule
Copy link
Contributor Author

ndhoule commented Jun 30, 2015

@boneskull Which lodash function would you use here? _.create?

@ndhoule ndhoule closed this Jul 5, 2015
@ndhoule ndhoule force-pushed the refactor/remove-dunderproto branch from a7cf327 to d5ddcdc Compare July 5, 2015 01:36
@ndhoule ndhoule reopened this Jul 5, 2015
@ndhoule
Copy link
Contributor Author

ndhoule commented Jul 5, 2015

@boneskull Refactored this to use lodash.create, which can easily be swapped out for a full lodash when necessary. I think this version is less expressive than the inherits version but meh

@boneskull
Copy link
Member

@ndhoule why do you feel it's less expressive?

@jbnicolai jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25
@jbnicolai
Copy link

@nathanboktae I agree
@boneskull create(..., {constructor: ...}) seems less expressive than inherit(..., ...).

On the other hand, since we're intending to go with lodash anyways, I say we merge this version :)

Perhaps to make this a bit nicer we could refactor it into a utility method in Base

function inherit(prototype, constructor) {
  return create(prototype, {
    constructor: constructor
  });
}

But no strong feelings about that.

@ndhoule
Copy link
Contributor Author

ndhoule commented Jul 5, 2015

haha pretty much exactly what @jbnicolai said, but really I don't care—I say merge it as is, it's certainly better than the __proto__ version

@jbnicolai
Copy link

Agree, still a huge improvement 👍

jbnicolai pushed a commit that referenced this pull request Jul 5, 2015
Replace __proto__ references with inherits module
@jbnicolai jbnicolai merged commit 2b2c576 into mochajs:master Jul 5, 2015
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

3 participants