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

Configurable proxy blacklist #774

Merged
merged 3 commits into from Sep 2, 2016

Conversation

lucasfcosta
Copy link
Member

This aims to close #765.

As brilliantly suggested by @meeber this adds a new Array containing keys which should be ignored when checking for non-existing properties on an assertion before throwing an error.

This PR contains:

  • An awesome and difficult replacement of a backtick which should have been an '
  • A new setting on chai.config called proxyExcludedKeys which receives an array of Strings representing the properties which should be ignored by the proxy on environments that support it
  • Docs explaining how it works, default values and other peculiarities
  • Tests checking:
    • Default values
    • If it does not throw an error on non existing then and inspect properties
    • If it throws an error on non existing then and inspect properties if they get removed from the proxyExcludedKeys array on environments that do support proxies

Let me know if I missed anything.

@keithamus
Copy link
Member

This LGTM (looks great to me). @meeber thoughts?

* User configurable property, defines which non-existing properties should be ignored
* instead of throwing an error if they do not exist on the assertion.
* This is only applied if the environment Chai is running into supports proxies and
* if the `useProxy` configuration setting is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple nits:

  • Line 78: Can remove "non-existing" since it's redundant with "if they do not exist on the assertion" later on in the same sentence.
  • Line 80: "running in" instead of "running into".

@meeber
Copy link
Contributor

meeber commented Aug 30, 2016

@lucasfcosta Awesome work, thanks for doing this! I just had a few nits related to comments. I really like how you handled the changes, particularly the tests with the helper function. You've done the world a great service by fixing that errant backtick.

@lucasfcosta
Copy link
Member Author

lucasfcosta commented Aug 31, 2016

Thanks for the help @meeber, since I'm not a native English speaker I make this kind of mistakes sometimes.
Now the comments really seem less redundant and more informative, thank you again for the input 😄

Let me know if I forgot anything.

* if the `useProxy` configuration setting is enabled.
* By default, `then` and `inspect` will not throw an error if they do not exist on the
* assertion object because we should not throw errors on Symbol properties such as
* `Symbol.toStringTag` nor on `.then` because it is necessary for for promise type-checking.
Copy link
Contributor

Choose a reason for hiding this comment

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

The part about Symbol properties and Symbol.toStringTag can remain in proxify.js because that's where the type check is; it's different than the configurable excluded keys. A new note needs to be added here about .inspect; it's ignored by default because that property is read by util.inspect, for example when using console.log on the assertion object.

@meeber
Copy link
Contributor

meeber commented Aug 31, 2016

@lucasfcosta I'm jealous of your ability to speak multiple languages!

I added a follow-up note above about the comments.

@lucasfcosta lucasfcosta force-pushed the configurable-blacklist branch 2 times, most recently from ca2a6d4 to 01220ce Compare August 31, 2016 22:39
@lucasfcosta
Copy link
Member Author

@meeber thanks for noticing that!
I have made the fixes you suggested and I also read the inspect utility and found some other minor typos, so I also fixed them.

@meeber
Copy link
Contributor

meeber commented Sep 1, 2016

LGTM!

@meeber meeber merged commit a42586e into chaijs:master Sep 2, 2016
@lucasfcosta lucasfcosta deleted the configurable-blacklist branch October 2, 2016 18:19
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.

Cannot console.log assertions because of proxies
3 participants