search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.47k stars 4.21k forks source link

Ember 3.17 helper invocation regression #18957

Open ilucin opened 4 years ago

ilucin commented 4 years ago

When doing this in a template:

<MyComponent @prop={{can.do}} />

ember is trying to invoke can helper instead of doing a controller property path lookup.

Regression happened in ember 3.17 and is still present in the latest beta builds. Works perfectly fine in 3.16.x.

Twiddle reproducing the bug: https://ember-twiddle.com/914c71225a4db0b2bb6fdbc8e79965fa

rwjblue commented 4 years ago

I think using this.can.do should avoid the issue (and avoid relying on the implicit this fallback which flagged by the no-implicit-this template lint rule).

Also, I am curious if in your specific scenario you happen to actually have a helper named can in your application? If so, the ambiguities are even worse, since using {{can}} in various positions would actually change what it means. Generally speaking our position is that a single reference should only ever refer to a single thing, but in this case (if you have a can helper) you are using {{can something}} (clearly a helper invocation) which means that {{can.blah}} should never refer to an implicit {{this.can.blah}} (because that would mean that {{can.do}} can be something different from {{can}}).

The issue here is that we actually do want to allow helpers to be invoked from path expressions (e.g. the contextual helper RFC), and the rendering engine update that initially landed in 3.17 did a bunch of plumbing work to make that possible. FWIW, we have discussed this specific scenario a bit on the core team, and the behavior change is limited to the MustacheStatements that are the value of a named argument in a component invocation which do not have any arguments themselves (basically, the exact scenario you reported). This is because it is somewhat ambiguous if you would be intending to pass the helper as a contextual helper or to invoke it immediately.

ilucin commented 4 years ago

@rwjblue I understand what you're saying here. It's just that this looked as an unexpected thing to happen after I tried to upgrade ember to 3.17. I believe it should have been documented somehow.

What led me to believe this is a bug is that it doesn't invoke a helper if I'm just trying to print out a value of can.do in the template like this {{can.do}} (not passing it as an argument to another component invocation).

Check out this twiddle - this output is pretty awkward if you ask me :)

locks commented 4 years ago

Thanks to both for the report and the discussion. While it's unfortunate you hit this corner case, I am marking as wontfix due to Rob's explanation, as well as the fact that we're moving away from property lookup fallback. @ilucin out of curiosity, did you get the property lookup fallback linting error in 3.16?

rwjblue commented 4 years ago

Just to be clear, I do think the failure feedback is particularly bad and we should totally work on an assertion that actually explains what is going on here to folks so they aren't hit with the same confusion.

ilucin commented 4 years ago

@rwjblue @locks thanks for jumping in here! This is not a regression obviously, just an opportunity to improve the documention, error assertions and maybe upgrade migration tooling.

Before closing this issue, can you please explain why is property lookup fallback happening when evaluating {{session.user.name}} statement and not when passing it as an argument to component invocation <MyComponent @prop={{session.user.name}} /> - twiddle example. Is this a bug? It's pretty confusing.

@ilucin out of curiosity, did you get the property lookup fallback linting error in 3.16?

I wasn't getting any linting errors for the property lookup fallback using the latest ember-template-lint version.

rwjblue commented 4 years ago

@ilucin out of curiosity, did you get the property lookup fallback linting error in 3.16?

I wasn't getting any linting errors for the property lookup fallback using the latest ember-template-lint version.

I wanted to make sure that this wasn't a bug in ember-template-lint, so I went ahead and made a test case to confirm that the usage in the issue description does indeed flag as a no-implicit-this failure. Mind reviewing my test cases over in https://github.com/ember-template-lint/ember-template-lint/pull/1333?

Can you double check your .template-lintrc.js config (it should be using the octane preset, or manually add 'no-implicit-this': 'error',)?

rwjblue commented 4 years ago

Before closing this issue, can you please explain why is property lookup fallback happening when evaluating {{session.user.name}} statement and not when passing it as an argument to component invocation <MyComponent @prop={{session.user.name}} /> - twiddle example. Is this a bug? It's pretty confusing.

Yeah, definitely is annoying. It is basically the same situation as your original report (the one with {{can.do}}). This is definitely a behavior change in 3.17. 🤔

@chancancode - What do you think? IMHO, this particular behavior change is somewhat unexpected (but as I mentioned before is a side effect of the work to make helpers "just a value").

chancancode commented 4 years ago

Yeah, I do think this is a bug. The contextual helper RFC is intended to be non-breaking. In particular, the Relationships with globals section laid out the transition path:

In the long term, we propose to unify the semantics such that globals will behave exactly like local bindings (i.e. we should make this second case work).

However, is not possible in the short term. This is due to the ambiguity between referencing a global variable and the property lookup fallback feature. We propose we simply wait until the property lookup fallback is fully deprecated and removed, at which point we can reclaim the syntax.

In the mean time, globals can be referenced explicitly using the component, helper, and modifier helpers.

So, in the meantime, things that used to work as property lookup fallback should continue to behave as such, and globals cannot be not true variables/values. Soon, we will add a deprecation for it here in addition to to the lint and perhaps in cases that would have accessed a property on a global variable, we can call it out in the same deprecation with that extra bit of information.

(In any case, even in that future world, can.do should be accessing the do property on the can global variable, instead of invoking the can helper, so either way that is probably still a bug in the implementation.)

We can fix this in glimmer-vm, but we probably have enough information to fix it here in an AST plugin also.

ilucin commented 4 years ago

Can you double check your .template-lintrc.js config (it should be using the octane preset, or manually add 'no-implicit-this': 'error',)?

@rwjblue You're right, no-implicit-this works fine. I wasn't extending octane preset in .template-lintrc.js - sorry for misleading you on this.

rwjblue commented 4 years ago

No worries!!

patocallaghan commented 4 years ago

@rwjblue This issue sounds like a dupe of https://github.com/emberjs/ember.js/issues/18788. I was having the same issue with local properties named “action” and “task”. Feel free to close my other issue if you agree.

While it was easy to workaround, in my mind it’s a regression because it’s a change in template behaviour and strict mode isn’t really official yet (besides the lint rule).

rwjblue commented 4 years ago

@patocallaghan ya, I mentioned above that I think this is a bug. I just don't think anyone has had a chance to dig in. My point RE: the linting rule was mostly just to point out that there is a fairly straightforward way to ensure that you have caught any locations that were accidentally relying on these behaviors (and therefore to fix your app), not that that makes this less important.