[BUGFIX beta] Remove chaining in Observable.set

[cibernox]

Ignore this for now

[stefanpenner]

yes please :P. Although I have no idea how to deprecate...

That being said, we can provide an add-on that restores this functionality for those who experience this as a pain point.

[cibernox]

For consistency, this also has to be done with setProperties (and maybe something else).

I'd been trying hard to test this but I just ember s is just broken for me, it hangs forever. I've nombom'ed everything and I still don't know what's going on.

[stefanpenner]

We may want to slide this into the current beta. If it turns out to cause lots of grief we can roll it back, but i suspect it wont, and for the ones who may, it is a mechanical change.

That way the 1.13 -> 2.0 upgrade statement of minimal pain remains true

@cibernox I'll look into your tests failure today.

[cibernox]

I'm kind-of blocked on this because I can't run the tests locally. ember s just freezes forever without any error message, and I've tried usual suspects (nombom'ed everything) in node 0.10 and ember still fails in node12/iojs :(

I'll try to reinstall node itself. Have you heard of this before?

[stefanpenner]

I'll try to reinstall node itself. Have you heard of this before?

currently you must still use node 0.10.35. Until we finish upgrading all the micro-libs to the latest transpiler.

[cibernox]

So the problem can come from using 0.10.38 ?

[stefanpenner]

Not a known problem.

The problem I am aware of is new node versions that have Symbols. Old traceur attempts to coerce them, which causes a hard crash.

Your problem appears in related?

[cibernox]

No, that's the problem with node 0.12/iojs. My problem is that ember s just hangs with no error message (and the process seems to be idle)

[stefanpenner]

No, that's the problem with node 0.12/iojs. My problem is that ember s just hangs with no error message (and the process seems to be idle)

i'll give it a try momentarily, just noming

[stefanpenner]

Unfortunately, I could not reproduce your issues. Everything appears to boot :(

let me see if i can fix your test failures though

[stefanpenner]

@cibernox for some reason i can't send you a PR, feel free to cherry-pick 48aad1d

[cibernox]

I couldn't cherry-pick it either, but I applied the changes as a .patch file

[cibernox]

Failed in sauce, probably random

[stefanpenner]

re-running

[stefanpenner]

likely needs to be bugfix beta

[rwjblue]

Looks like tests are fixed up.

@stefanpenner - Have time for a final review?

[stefanpenner]

@stefanpenner - Have time for a final review?

I'm good with this.

Lets bring it up at this weeks meeting.

[stefanpenner]

resolution:

• lets land it in this beta cycle
• someone needs to write a static analysis to warn and report (@mmun may be interested)
• blog post entry for the beta 1.13 Release Blog Post #11254
  • need to explain why
  • need to explain how to check + upgrade.

[cibernox]

I'm very curious about the static analysis. I really don't see a reliable way of warn about this.

[stefanpenner]

I'm very curious about the static analysis. I really don't see a reliable way of warn about this.

just the obvious

o.set().set() or
o.set().<anything>
o.setPropertyes().<anything>
var a = o.set();

[cibernox]

@stefanpenner that analysis be done as part of the build process in ember-cli?

[mmun]

@cibernox Basically you want to write a babel transform that looks for .set()'s being used as a value, specifically you want to look for CallExpressions with callee.property.name === 'set' and whose parent node is not an ExpressionStatement.

[mmun]

@cibernox Yes, we'll build it into ember-cli, but likely also build a node module for non-ember-cli projects to use.

[cibernox]

It's amazing the kind of things you can do with esprima. Completely dark magic to me.

[fivetanley]

Maybe we could start an ES-lint plugin package for ember? http://eslint.org/docs/developer-guide/working-with-rules.html

[fivetanley]

Ember Watson is also a great place to start if you're new to these kind of transforms using recast, but it sounds like we want a warning instead of an actual transformation. https://github.com/abuiles/ember-watson

[cibernox]

Rebased

[mixonic]

@stefanpenner @cibernox @mmun is this going into beta? It would need to land asap. I'll happily doc it in the blog post, but I will need guidance.

[cibernox]

This should go into beta to align ember get/set with regular ES5 getters/setters, the controversy here is how much of an issue is breaking backwards compatibility, since there's no easy way of deprecate this.

On the other hand, very very few people (if any at all) will be using such an awkward construct like obj.set('foo', bar).set('baz', qux).set('foobar', bazqux)

[mixonic]

@cibernox master is currently 2.0- should it just go into 2.0 as a small breaking change?

[cibernox]

Personally I'm ok with breaking changes in general as long as they are small and the upgrade path is properly documented.

[stefanpenner]

i am +1

[mixonic]

Merging this into beta (Ember 2.0)

[stefanpenner]

👍

[rwjblue]

Pulled into beta.

[cibernox]

Does this deserve an special entry somewhere in the website repo?

[mixonic]

@cibernox I would just like to add it to the 2.0 "breaking changes" section of the blog post: emberjs/website#2212

[Kerrick]

This broke apps in the wild but still got merged, and the breaking change never got documented--not on the blog, not in the deprecations section, just a silent update to the API docs. I had to read the git the history of set_properties.js and track down these discussions. I'd still like to see this documented somewhere.

[Kerrick]

For those who come here after this, I've released an addon that undoes this change! Hope you find it useful: https://github.com/secondstreet/ember-observable-set-chaining

[cibernox]

@Kerrick I'm sorry this wasn't documented properly. Maybe it's not too late to add a note in the 2.0 blog post.

Also, deprecate this functionality was almost impossible, since the only way of detecting chaining would be some sort of static code analysis.

However I'd discourage you to restore the old functionality with an addon. The same way this change broke your app, it's likely that undoing it will break addons in the wild, ending up in your app being broken anyway.

[cibernox]

@Kerrick I've added a not as the very first line of the changelog for the 2.0 release with big a breaking label, so at least this is easier next time.

[Kerrick]

I've added a not as the very first line of the changelog for the 2.0 release with big a breaking label, so at least this is easier next time.

@cibernox Thank you! If anybody else besides us still hasn't upgraded to 2.X and they read the changelog, hopefully they won't be confused by this now. 👍

However I'd discourage you to restore the old functionality with an addon. The same way this change broke your app, it's likely that undoing it will break addons in the wild, ending up in your app being broken anyway.

Yeah, but the only way to find all instances of this to change them across our six Ember apps and 25K+ lines of JavaScript would be some sort of static code analysis. 😝