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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor utils.addChainableMethod helper #900

Merged
merged 3 commits into from Jan 12, 2017
Merged

Refactor utils.addChainableMethod helper #900

merged 3 commits into from Jan 12, 2017

Conversation

shvaikalesh
Copy link
Contributor

Use Object.setPrototypeOf instead of __proto__

  • Object.setPrototypeOf is more strict than __proto__: it throws on failure instead of being silent.
  • __proto__ is in Annex B of the spec; using it is kinda discouraged.

This annex describes various legacy features and other characteristics of web browser based ECMAScript implementations. All of the language features and behaviours specified in this annex have one or more undesirable characteristics and in the absence of legacy usage would be removed from this specification.

  • I would avoid relying on early implementations of __proto__ (before it was defined on Object.prototype) .
  • Object.setPrototypeOf reads better 馃槃.
  • Every browser we support, has either both __proto__ and Object.setPrototypeOf or neither of them.

When we drop IE < 11, we would simplify addChainableMethod even more.

Remove hardcoded non-configurable function keys

They used to cause troubles in ancient IEs, but work fine in IE.

@meeber
Copy link
Contributor

meeber commented Jan 11, 2017

LGTM

@shvaikalesh
Copy link
Contributor Author

I've updated the PR with perf improvement:

We get list of non-configurable props on functions only once and then only indexOf in hot code.
Internet Explorers would love that.

@meeber
Copy link
Contributor

meeber commented Jan 11, 2017

Seems like a good improvement. Still LGTM.

@vieiralucas
Copy link
Member

vieiralucas commented Jan 12, 2017

Very good explanation 馃槃
LGTM

Copy link
Member

@lucasfcosta lucasfcosta left a comment

Choose a reason for hiding this comment

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

LGTM!

@lucasfcosta lucasfcosta merged commit c5ca76c into chaijs:master Jan 12, 2017
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