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

Compare with null instead of strict with undefined #26

Closed
wants to merge 1 commit into from

Conversation

Kikobeats
Copy link

No description provided.

@KyleAMathews
Copy link
Collaborator

@Kikobeats hey! I'd love to make you a collaborator so you can merge this and other PRs and roll a new release! Please signup over at #25 and let's make this happen!

@TehShrike
Copy link
Owner

This change causes some tests to break. What's the motivation for this change? Overwriting null values in the target would be a breaking change. Is this a common use case?

@Kikobeats
Copy link
Author

Actually I didn't remember... but basically compare with == null is the way for determinate if somehting exists (it means, if the value is null or undefined, it doesn't exist)

function exists(v) { return v != null }

I think that I tried to express something like, but if this breaks tests (how do you know that breaks tests if travis integration isn't enabled? :P) feel free to close that.

@TehShrike
Copy link
Owner

I checked out your branch locally and ran the tests.

I know what the == null check does, matching both null and undefined - but those are two different values, so starting to treat them both as the same thing in this case could have unforeseen consequences.

At the very least we should probably have a couple new tests that show off the problems that this would solve.

@TehShrike
Copy link
Owner

oh, by the way, I know the PR was originally opened a while ago, so you may not remember the reasoning that prompted this change - no big deal, you can feel free to close this PR and open another issue or PR later if you remember the use case that needed this change. <3

@TehShrike TehShrike closed this Sep 29, 2016
@Kikobeats Kikobeats deleted the patch-1 branch September 29, 2016 07:24
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.

3 participants