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

Revert #3218, replace #4358. Deprecate global access from templates #4459

Merged
merged 1 commit into from
Jul 25, 2014

Conversation

mixonic
Copy link
Member

@mixonic mixonic commented Mar 2, 2014

Replaces #4358. These deprecations are narrow in scope, and only impact {{Global}}. They do not deprecate any other global access in templates. Deprecating all global access from templates would a) need to be done helper-by-helper or b) would impact the get and binding code at a lower level.

@xtian I'm rewritten your first commit but preserved it here.

@ebryn yo.

@mixonic
Copy link
Member Author

mixonic commented Mar 2, 2014

Bleh, this is probably not actually ready to go. There are many deprecation notices that fire in the logs during a test run now, an example:

Global lookup of TemplateTests.LabelView from a Handlebars template is deprecated.
Global lookup of Ember.Handlebars.EachView from a Handlebars template is deprecated.
Global lookup of Foo.bar from a Handlebars template is deprecated.
Global lookup of TemplateTests.CollectionView from a Handlebars template is deprecated.
Global lookup of TemplateTests.YieldingView from a Handlebars template is deprecated.

Looks like {{#view calls are impacted by that first commit. Opinions wanted: Should we clean up the usage of global view helper in tests, or not deprecate that behavior?

@xtian
Copy link
Contributor

xtian commented Mar 2, 2014

Sorry for being MIA and thanks for taking this over. This is a weird issue and I wasn't sure what to do with it.

#3218 was a small change that made {{Foo}} behave the same as all the helpers that use bindings (i.e., always look up Foo as a global).

I tried to explain in #4358, but it is not simple to invert that behavior. Many helpers use globals as you've seen, and most use bindings directly. There's currently no way to determine whether a binding is being created from a template vs app code.

I didn't think modifying the bindings code was great, but it was the only solution I could find. Anyway, I was missing something and didn't know how to get the tests to pass.

I do think only deprecating global lookup for {{Foo}} and {{#each Foo}} makes things more inconsistent because there are many other cases where global lookup can happen.

@mixonic
Copy link
Member Author

mixonic commented Mar 2, 2014

@xtian definitely agree about deprecations being inconsistent only on lookup and #each. I think we could cover the other helpers without too much effort if that level of deprecation is what is needed here. @ebryn asked me to add these deprecations, but I'm not clear myself on what the long-term plan is (is it to remove global access from templates post 1.x?).

I prefer keeping this discussion focused on the templates though, and to avoid discussing anything lower-level until #4124 can be merged (since it may raise deprecations for some of the inconsistencies as well).

@stefanpenner stefanpenner mentioned this pull request Mar 7, 2014
8 tasks
@mixonic
Copy link
Member Author

mixonic commented Mar 19, 2014

@ebryn this deprecates direct global access and access via #each. It does not deprecate calls to resolveParams like the original, but I don't think that is required.

@oskarols
Copy link
Contributor

@mixonic Do you mean it does not fix the issue where uppercase properties are resolved as globals?
I.e. having controller#NBAPlayer and using that with contentBinding=NBAPlayer when using a component in a template.
If so, should that be in a separate issue?

@mixonic
Copy link
Member Author

mixonic commented Mar 30, 2014

@oskarols I'm confused by your example. This PR is only for deprecation of global access. Via {{Global}} and {{#each Global}}. No other access is changed.

@wagenet wagenet added the discuss label Apr 2, 2014
@mixonic
Copy link
Member Author

mixonic commented Apr 9, 2014

Newly rebased, I believe this is ready to go. /cc @ebryn this was something you asked for, let me know what you think of the implementation.

@mixonic
Copy link
Member Author

mixonic commented May 14, 2014

@ebryn monthly yo. I could use feedback or a merge or a close. Freshly rebased.


if (value === undefined && root !== Ember.lookup && isGlobalPath(path)) {
if (isGlobalPath(path)) {
Ember.deprecate("Global lookup of "+path+" from a Handlebars template is deprecated.", !options.callGlobalDeprecation);
Copy link
Member

Choose a reason for hiding this comment

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

This deprecation shouldn't be fired if the path resolved locally. It's legit to have capitalized local paths. The behavior we're looking for is described here: #3218 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not possible with how get works today. Get does the fallback internally. Short of calling get with the same path and Ember.lookup as the first argument, imo a deprecation as specific as you want won't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, we have normalizePath. Maybe that can help.

@mixonic
Copy link
Member Author

mixonic commented Jun 23, 2014

This is pretty good. Still a more deprecations firing from tests than I would prefer, but this is pretty close to ideal.

rwjblue added a commit that referenced this pull request Jul 25, 2014
Revert #3218, replace #4358. Deprecate global access from templates
@rwjblue rwjblue merged commit ccf7b03 into emberjs:master Jul 25, 2014
@rwjblue rwjblue deleted the handlebars-global-path branch January 3, 2015 21:00
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.

6 participants