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

[BREAKING BUGFIX] Do not assume null Ember.get targets are all globals #3852

Merged
merged 1 commit into from
Mar 8, 2015

Conversation

mixonic
Copy link
Member

@mixonic mixonic commented Dec 4, 2013

Some bugfixin'.

  • Changes globals to only mean properties starting with $ or a
    capital letter.
  • Changes normalizeTuple(obj, 'Foo') to resolve Foo as a global. This makes the behavior consistent with normalizeTuple(obj, 'Foo.bar') which already resolves Foo as a global (and looks up bar).
  • Changes normalizeTuple(null, 'Foo') to resolve Foo as a global.
  • Specs normalizeTuple('null', 'foo') to resolve to [undefined, ''].
    This is unfortunate but needed to keep getPath happy.

This code is old, and tricky to modify due to perf optimizations and edge case behavior. It would be great to refactor more heavily at some other date.

Fixes #3760

/cc @bantic @hejld

@wagenet
Copy link
Member

wagenet commented Dec 8, 2013

I think we want to move this direction, but we also want to make sure we don't break existing apps. We'll have to think about this more.

@mixonic
Copy link
Member Author

mixonic commented Dec 8, 2013

Tricky no doubt, but the code in master has some weird inconsistencies this cleans up. The biggest change is in normalizeTuple, but I doubt many apps consume that API. It doesn't even seem like it should be exposed, really.

Examples of bad behavior in master:

// Complex paths are normalized differently than simple paths
Ember.normalizeTuple(null, 'SomeGlobal.aProp'); //=> [SomeGlobal, 'aProp']
Ember.normalizeTuple(null, 'SomeGlobal'); //=> throws Invalid Path

Ember.normalizeTuple({some: 'obj}, 'SomeGlobal.aProp'); //=> [SomeGlobal, 'aProp']
Ember.normalizeTuple({some: 'obj}, 'SomeGlobal'); //=> [{some: 'obj'}, 'SomeGlobal']

window.koo = { joo: 'soo' };
Ember.normalizeTuple(null, 'koo'); //=> throws Invalid Path
Ember.normalizeTuple(null, 'koo.joo'); //=> [{joo: 'soo'}, 'joo']

// Non global paths are still treated as globals
Ember.get('length'); //=> the number of frames on the window
Ember.get(null, 'length'); //=> the number of frames on the window

Looks like I failed to clean up the names of several tests, pushing a fix for that shortly.

@stefanpenner
Copy link
Member

👍 on this, but I share @wagenet's concern we will need to be thorough on this one.

@ebryn
Copy link
Member

ebryn commented Dec 29, 2013

I think we should probably first add some deprecation warnings in the dev builds so we can ween people off the bad practices. That would help us also collect some additional tests for valid use cases that we might not have tests for.

@mixonic
Copy link
Member Author

mixonic commented Jan 3, 2014

@ebryn I suggest another PR that raises deprecation warnings for null-target-non-global get calls, a with-target-but-global-path normalizeTuple calls. Sound right?

@mixonic
Copy link
Member Author

mixonic commented Mar 19, 2014

Rebased on top of the ES6 branch and @stefanpenner's perf work

@mixonic
Copy link
Member Author

mixonic commented Apr 9, 2014

Rebased again and tests passing. Still blocked on #4124.

mixonic added a commit to mixonic/ember.js that referenced this pull request Jul 8, 2014
…3852

PR emberjs#3852 changes some edge case behavior for get and normalizeTuple. Ahead
of those changes, this commit introduces deprecation notices.

  * Deprecate get for local paths on global contexts, only if they
    return data.
  * Deprecate normalizeTuple calls that return a non-global contenxt
    and a simple global path.
@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2014

The deprecations from #4124 and #4459 will be in the 1.8 beta series (ships in 3 weeks), I think this is probably good to merge just after 1.8 is branched. Any objections?

Will need to be rebased (and get the tests passing) before that though.

@mixonic
Copy link
Member Author

mixonic commented Jul 31, 2014

Rebased. Yet another minor violation of semver from my hands if it goes into 1.x.

@mixonic
Copy link
Member Author

mixonic commented Jul 31, 2014

Moved validateIsPath exception into its own function to keep AST size of normalizeTuple down, per @stefanpenner's suggestion.

@wagenet
Copy link
Member

wagenet commented Nov 1, 2014

@rwjblue can we merge now?

@rwjblue
Copy link
Member

rwjblue commented Nov 2, 2014

@wagenet - Yes, I think so.

@mixonic - Can you rebase and add a [BREAKING BUGFIX] Changelog entry?

@mixonic mixonic self-assigned this Nov 2, 2014
@rwjblue
Copy link
Member

rwjblue commented Jan 3, 2015

@mixonic - Needs a rebase, but we agreed that this was good a couple F2F meetings ago (in NYC).

@mixonic mixonic force-pushed the get branch 3 times, most recently from 2d298e8 to d33ecd8 Compare March 8, 2015 01:55
@mixonic mixonic changed the title [BUGFIX] Do not assume null Ember.get targets are all globals [BREAKING BUGFIX] Do not assume null Ember.get targets are all globals Mar 8, 2015
@mixonic mixonic force-pushed the get branch 2 times, most recently from bd0fe7f to 1a3dde2 Compare March 8, 2015 02:06
* Changes globals to only mean properties starting with $ or a capital letter.
* Changes normalizeTuple(obj, 'Foo') to resolve Foo as a global. This
  makes the behavior consistent with normalizeTuple(obj, 'Foo.bar') which
  already resolves Foo as a global (and looks up bar).
* Changes normalizeTuple(null, 'Foo') to resolve Foo as a global.
* Specs normalizeTuple('null', 'foo') to resolve to [undefined, '']. This
  is unfortunate but needed to keep getPath happy.

This code is old, and tricky to modify due to perf optimizations and
edge case behavior. It would be great to refactor more heavily at some
other date. I've spec'd out more of the current behavior and edge cases
in this commit.

Fixes emberjs#3760
@mixonic
Copy link
Member Author

mixonic commented Mar 8, 2015

@rwjblue this passes, and I've updated the changelog. Let me know if you had something else in mind.

Excited to close this before it sits open for two years. This is probably the longest span of time between a proposed software change and its realization I have ever authored :-)

@rwjblue
Copy link
Member

rwjblue commented Mar 8, 2015

ZOMG ZOMG ZOMG

rwjblue added a commit that referenced this pull request Mar 8, 2015
[BREAKING BUGFIX] Do not assume null Ember.get targets are all globals
@rwjblue rwjblue merged commit f405cbd into emberjs:master Mar 8, 2015
@rwjblue rwjblue deleted the get branch March 8, 2015 16:19
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.

[BUGFIX] Ember.get returns 'length' of null
5 participants