Change up componentName

[evenstensberg]

Last PR was messed up. Sorry about that. Also, sorry for waiting, had to git here and there. Cheery-picking skills aren't that great yet. You'd end up with 9 file changes if I'd PR the fix I ended up with, so I just forked it lazily. Enough about that...

So, the PR.. Lot's of confusion here.

First of all, I didn't see the validation before, so thanks for sorting that out. To address the reason for this PR:

1. Want to get rid of componentName = componentName || 'String'
2. Swap it around like a conditional, means that we can get rid of the self assignee here. I don't like it. Repeating code should be no-no.
3. The loop itself needs to be checked, what do you think of this method?

Also thank @sampeka for pointing out clearly bad code. The previous PR wasn't really well composed.

Ultimately I'd love to have componentName = 'String' and have it loop through like I did in this PR.

Cons/Pros? @ryanflorence

[taion]

What is this supposed to do?

This always assigns {} to componentName, and the function as a whole will always return {}, as @ryanflorence noted.

[ryanflorence]

Want to get rid of componentName = componentName || String

Why? Is this just a style thing? or is there a bug or something?

[timdorr]

Thanks for this @ev1stensberg, but there aren't bugs (that we know of) in this code as-is and I don't think there is any point here other than a style change.

[evenstensberg]

@ryanflorence For me it just seems like using logical OR with itself is messy to be able to use another value on demand. I have no problems using locial OR, and many use it like that, but I guess this is code style related. @taion to address the use of {} just wanted to make sure I didn't break anything while coming up with the suggestion, as I needed feedback on what you think about this!

Would it make sense to get rid of locial OR?

[taion]

No – it's generally accepted best styling practice: https://github.com/airbnb/javascript#comparison--unneeded-ternary.

[evenstensberg]

Wasn't targeting using tenaries, but I assume you knew that. What we should have, is something like:
myVariable || = 'String'

#ES8proposal

[evenstensberg]

I'll slowly start on a temp fix on this btw. Would be as methods/functions though.
Conceptional it would be something like myVariable.equalOr(String). I'm sure there's a polyfill for doing such, @taion do you know of any?

[taion]

There really isn't anything that needs to be fixed here. This is a commonly used pattern, e.g. in https://github.com/facebook/react/blob/v15.0.1/src/isomorphic/classic/types/ReactPropTypes.js#L114.

[evenstensberg]

Cool if I put up a Issue once I've created something showing how I'd pull this off?

[evenstensberg]

@taion @ryanflorence @timdorr

This is what I wanted to target. Would be such a nice thing to have a logical my variable || = 'Heyho' so we avoid this.

Note: This issue is made by babel, but I wanted to encapsulate the effect & show it what it meant to reuse this in an example.

[timdorr]

You'll never hit that error. It's logically dead code.

[edvinerikson]

There is default parameters in ES6 which can be used instead but if it isn't used in other places in the code it seems strange to me to change this line of code.

[evenstensberg]

@timdorr In what way?
@edvinerikson tenaries are slow compared to logical OR.

[timdorr]

@ev1stensberg Show me an input where that function will throw an error.

[evenstensberg]

@timdorr thought you meant the function. The error is just there to ensure the user passes something into assignOr(). If it's wrong, how would you change it up?

[timdorr]

But it comes after the logical OR. Again, show me any input that will cause it to throw that error. You cannot. Therefore, there is no point to it existing.

I'm really confused why you're so interested in this one line of code in a minor function. Is there some bug here that we're missing? This seems like a lot of bikeshedding.

[evenstensberg]

say a user has var listItem = listItem || 'No listItem provided, oh buggers!' Seems to me this is repetitive code. Meaning listItem should be unique and you should not apply more values to it through the OR.
As stated in the example given, according to how I set it up: var listItem = listItem.assignOr("string") this could be reversed though, having you pass in an object as an argument rather than a string, as in the example. It would then be var var someString = someString.assignOr(obj)

Iknow this is like super-picky. You'd end up having the same repetitive code just cause you assign a method to it. For me, this makes more sense than redeclaring your object. This is a paradox though, it's a hacky way of doing the same thing. That's why var buggersObject || = someString would be much better in my eyes.

@timdorr example is twisted in as explanations here. obj is the argument passed to your assign, the conditional was meant to catch a error if user types assignOr() instead of having a parameter. Sure I could have used ...args to do that though.

[evenstensberg]

Additional note to my cyborg typo: Did you get any of that? If this just seems like you said, bikeshedding, I'd agree to that. Just wanted to point out why this is bad versus what I think it should be || =

[timdorr]

I'm not seeing the point at all. Just because you use a variable twice doesn't make it repetitive. You still have to use it twice in your example. This only muddies the code further because now you've over-abstracted a simple conditional assignment.

Honestly, we might make better use of our time discussing the merits and performance benefits of single- vs. double-quoted strings...

[evenstensberg]

@timdorr Yeah I mentioned that in what I wrote, both about abstraction and about repetitiveness. Anyways, enjoy rest of the week! 👊

[taion]

1. Branches are effectively free unless mispredicted; the ternary v logical boolean thing is just a stylistic choice, not a perf thing.
2. ES6 default parameters have slightly different semantics, in that the default will only replace undefined, not null (or other falsy values, but those matter less) – so unfortunately while they are the cleanest choice, they're not a drop-in replacement.
3. What @timdorr said in Change up componentName #3330 (comment) – there's nothing wrong with repeating code per se, especially if it's in the context of a common idiom. In practice this sort of pattern doesn't really lead to incremental bug load.

[evenstensberg]

@taion Thanks for the fix on tenaries. I'm not saying var hey = hey || 'ho' is bad programming practice. That would be absurd as 90% use it to set a fallback default value. I'm just pointing out, to see the variable value first evaluate it's own value is in my eyes not something we , I want.

One thing to say about default parameters, is that it's not fit for everything, especially when it sets undefined and not null. In this use case the default would be a string, so setting the type or object
would seem like a shady thing to do, as we want to initialize this as a string, but not set it to undefined. (In my sunny world)

I'm not the best programmer, of which you'd see in one of my many PR's. Worth noting.

Anyways, think this discussion has taken it's latest breath for now, thanks for participating all!