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

Rendering <button> in each loop breaks application #16826

Closed
GendelfLugansk opened this issue Jul 17, 2018 · 44 comments
Closed

Rendering <button> in each loop breaks application #16826

GendelfLugansk opened this issue Jul 17, 2018 · 44 comments
Assignees

Comments

@GendelfLugansk
Copy link

In ember 3.3.0 following template breaks application:

{{#each buttons as |button|}}
  <button>{{button}}</button>
{{/each}}

buttons is a simple array. In console I can see Uncaught Error: unreachable. This error does not happen with other tags (i.e. works fine) and does not happen on ember 3.2.2

Repository with demonstration: https://github.com/GendelfLugansk/ember-rendering-bug

@nightire
Copy link
Contributor

Yeah, confirmed, we've encountered same error after upgrade from 3.2.2 to 3.3.0

@rwjblue
Copy link
Member

rwjblue commented Jul 17, 2018

I'm somewhat surprised that this broke in 3.3, but I would have expected it to break in 3.4 (due to the angle bracket invocation feature).

I'll try to dig in to confirm if it is related...

@rwjblue
Copy link
Member

rwjblue commented Jul 17, 2018

Changing to:

{{#each buttons as |text|}}
  <button>{{text}}</button>
{{/each}}

Should resolve the issue (assuming that my guess RE: angle bracket invocation feature is correct).

@GendelfLugansk
Copy link
Author

@rwjblue yes, changing from button to text or btn helps. Thank you for explanations.

@nfc036
Copy link

nfc036 commented Jul 18, 2018

Just an informational message: I had the same problem with {{#each model as |img|}}. Changing img to i.e. "foto" resolves this error. Is this breaking change (ember-source 3.2.2 => 3.3.0) documented somewhere?

@nightire
Copy link
Contributor

nightire commented Jul 18, 2018

Just another information: this issue tricks me a lot, the key is: don't use html tag name as the as part of each any helper that yields something.

{{#each people as |p|}} {{!-- don't, because `p` is a html tag name --}}
  {{p.name}}
{{/each}}

{{#each people as |person|}} {{!-- this is fine --}}
  {{person.name}}
{{/each}}

I'm not sure if it will be a breaking change, but I hope not.

@GendelfLugansk
Copy link
Author

GendelfLugansk commented Jul 18, 2018

@nightire as I can tell, we can use html tag name in as part if that html tag is not used inside block. I also think it affects not just each but any block helper that yields something, including our own components.

@rwjblue
Copy link
Member

rwjblue commented Jul 18, 2018

Confirm, yielding any block param that will be used as an angle bracket invocation will result in this issue.

See the following issues for some more background information:

@rwjblue
Copy link
Member

rwjblue commented Jul 18, 2018

This change should not have happened during 3.3, and we will try to figure out why it did (and fix it). However, we do intend for it to land as part of 3.4 (along with support for linting in applications as a general guide).

@lone-cloud
Copy link

lone-cloud commented Jul 18, 2018

Should we be expecting for this to be patched in 3.3.1?

@rwjblue
Copy link
Member

rwjblue commented Jul 18, 2018

Probably, but its still a good idea to refactor away from the patterns that are currently breaking.

The ember-template-lint rule no-shadowed-elements is a great way to enforce this.

@mmun mmun changed the title Rendering <button> in each loop breals application Rendering <button> in each loop breaks application Jul 19, 2018
@lougreenwood
Copy link

I also started getting error: unreachable with this add-on in 3.3...

https://github.com/tedconf/ember-collapsible-panel/issues

@samselikoff
Copy link
Contributor

Is this considered a breaking change? I'm also a little confused by the RFC as to whether this is allowed (and supposed to work), or allowed at build-time (but breaks at run-time).

@rwjblue
Copy link
Member

rwjblue commented Jul 27, 2018

Is this considered a breaking change?

Yes, it is a breaking change and we are working on the fix.

I'm also a little confused by the RFC as to whether this is allowed (and supposed to work), or allowed at build-time (but breaks at run-time).

The current behavior matches what the RFC stated (essentially that a block param named button + <button></button> in the blocks template, would assume that the block param was a yielded closure component and attempt to render it as such), but based on feedback (here and in other issues) we determined that the result is definitely something we consider a breaking change.

The current plan (after discussing with the core team on 2018-07-20) is:

  • Ensure that all block params that "shadow" normal HTML elements inside their blocks, continue to render the HTML element (and do not assume they should "invoke" a yielded component).
  • Issue a deprecation when this scenario is detected (with until: '4.0.0')
  • Ensure the linter is setup for newly generated Ember apps that will lint against using confusing block param names (where the block param could be an HTML element name, and that HTML element is invoked in the blocks template).
  • Backport these fixes to Ember 3.3

@nerdybeast
Copy link

I have been having this same issue for the last couple of days as well trying to figure out what was going on:

<select>
	{{#each options as |option|}} <!--I see now the issue is I named this "option" a valid html tag -->
		<option value="{{option.id}}">{{option.name}}</option>
	{{/each}}
</select>

Changing to this fixed the problem for as well:

<select>
	{{#each options as |opt|}} <!-- Renamed to "opt" -->
		<option value="{{opt.id}}">{{opt.name}}</option>
	{{/each}}
</select>

Just throwing this out there for more documentation on the issue.

@rwjblue
Copy link
Member

rwjblue commented Aug 6, 2018

Just throwing this out there for more documentation on the issue.

👍 thanks for that (and sorry for the issue)

@cafreeman
Copy link
Contributor

@rwjblue I'm not actually ables to find evidence of the no-shadowed-elements template lint rule in https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rules.md

Does it exist somewhere else?

@nlfurniss
Copy link
Contributor

@cafreeman it's not listed there (though it should be), but the rule is here

@cafreeman
Copy link
Contributor

Ah, can't believe I missed that. Thank you!

@robclancy
Copy link

The no-shadowed-elements rule isn't very descriptive, ended up here thinking the rule was another issue just with formatting (in vscode with the ember addon there is no difference between formatting issues and errors, I think with the linter too?).

My question is will this have a better error in future? Not everyone will be running the linter and I can imagine coworkers running into this and not knowing what's going on when there error isn't useful.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 29, 2019

Can anyone reproduce this on the latest stable release of Ember?

@GendelfLugansk
Copy link
Author

@pzuraq yes, I can reproduce this error using 3.8.0

@Techn1x
Copy link

Techn1x commented Apr 24, 2019

Just to make things incredibly confusing, this can happen (it happened to me)
Running Ember 3.4

{{#each someArray as |item i|}}
  {{#if (gt i 0)}}
    {{fa-icon item.icon}}
  {{/if}}
{{/each}}

Even though I wasn't using an <i>, the {{fa-icon}} component (from ember font awesome) creates one, and this caused the fun Uncaught Error: unreachable error

Also, this isn't caught by ember-template-lint's no-shadowed-elements, making it harder to find and diagnose :(

@rwjblue
Copy link
Member

rwjblue commented Apr 24, 2019

I can absolutely see how that would be pretty confusing. Internally ember-font-awesome modifies the output template and replaces the {{fa-icon component invocation directly with <i> which is the thing that caused the error you hit.

RE: this not being caught by no-shadowed-elements, I think we could do a bit of work in the ember-font-awesome AST transform to ensure that this specific scenario doesn't exist (basically error or automatically rewrite any block params named i while transpiling).

@lifeart
Copy link
Contributor

lifeart commented Apr 24, 2019

@robclancy
Copy link

robclancy commented Apr 24, 2019

We have dynamic fa-icon all over the place, it does work. Pretty sure what you linked will have had ember/glimmer already turn it into a string. Otherwise there is no chance we would even use the package.

You can also send in an object which has an iconName and prefix set.

@Techn1x
Copy link

Techn1x commented Apr 25, 2019

Sorry - whether or not dynamic icon names work, that's just an issue with the example I gave, but the point still stands.

The basis of that particular example (using i as a block param, and fa-icon in the inner block) is used in a non-trivial amount of places throughout my codebase - I ended up renaming all block param uses of i to index to be sure

Also worth noting there's two ember font awesome projects going, depending on the FA version you're using. They probably have differences in what they support etc.
https://github.com/martndemus/ember-font-awesome
https://github.com/FortAwesome/ember-fontawesome

@Caltor
Copy link

Caltor commented May 22, 2019

Is this issue still being looked at? it appears to be nearly a year since it was reported but I encountered the

Uncaught Error: unreachable

message today with the following code in Ember v3.4.4 (which I believe is a current LTS version).

<select>
    {{#each options as |option|}}
        <option value={{option.id}}>{{option.name}}</option>
    {{/each}}
</select>

P.S. I've since changed the code to:

<select>
    {{#each options as |opt|}}
        <option value={{opt.id}}>{{opt.name}}</option>
    {{/each}}
</select>

which obviously works.

@lifeart
Copy link
Contributor

lifeart commented May 22, 2019

@Caltor latest LTS - 3.8 https://emberjs.com/releases/

@Caltor
Copy link

Caltor commented May 22, 2019

@lifeart it also says 3.4 bugfixes until May 2019.

@rwjblue
Copy link
Member

rwjblue commented May 30, 2019

3.4 will only be receiving security fixes at this point, 3.8 will receive bugfixes until 3.12 is promoted to LTS

@Caltor
Copy link

Caltor commented Jun 10, 2019

@rwjblue So LTS actually means "we ignore the bug until the LTS period has elapsed"? Interesting. I would have thought bugs spotted within the LTS period would be fixed. Still I understand it's a community project etc etc and it's no big deal to workaround.

@lifeart
Copy link
Contributor

lifeart commented Jun 10, 2019

@Caltor #16826 (comment) there is workaround for this case, and workaround pretty clear and it's following official linting rules https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-shadowed-elements.md

Copy link
Member

rwjblue commented Jun 10, 2019

@Caltor - I am sorry that we didn't get this fixed for the LTS. 😩

The main issues here are that it has turned out to be gnarly to fix (I have spent a few hours on it), and in nearly all cases updating to avoid the conflict makes your code better and easier to understand which generally makes fixing this particular bug a bit lower priority than others that are actually blocking folks.

@Caltor
Copy link

Caltor commented Jun 11, 2019

@rwjblue No worries! If the linter picks it up then that is good. It is only a result of bad coding anyway.

@BobrImperator
Copy link

BobrImperator commented Jul 16, 2019

This issue also happens when a yielded components hash has the same name as helper, and linter doesn't catch that.

<AngleTable as |t|>
  <t.header />
  {{t "unreachable"}}
</AngleTable>

@pzuraq
Copy link
Contributor

pzuraq commented Jul 16, 2019

@BobrImperator I'm not sure that problem is something that is possible to catch ahead of time, and it should have always been a failure because local variables have always taken precedence over globals. The reason we can't catch it ahead of time is similar to why you can't lint against something like this in JS:

function foo() {

}

function bar(foo = 'a string') {
  foo(); // Error: foo is not a function
}

While it's reasonable to assume that this is a shadowing mistake on the developer's part, because of JS's dynamic nature, we can't know for sure that foo is not a function when being passed into bar. If we were using a typed language instead, then we may be able to, but unfortunately I don't think we can currently.

@BobrImperator
Copy link

I kind of expressed myself badly, I don't really expect this to be caught beforehand.
But if I remember correctly, there's a compile error for action helper when it's overwritten.
I don't remember the exact syntax, but it should be something like this, similar but with action.

<AngleTable as |action|>
  <t.header />
  {{action "unreachable"}}
</AngleTable>

it could be a different case though

@cah-brian-gantzler
Copy link

cah-brian-gantzler commented Dec 31, 2019

FYI just spent a couple hours on debugging til I finally googled and found this issue (was using option and the yielded name) so its still out there on 3.15.

What was weird was that {{log option}} reported the correct value inside the loop, so never occurred to me the name of the variable could be the problem

Matthijsy added a commit to csvalpha/amber-ui that referenced this issue Feb 27, 2020
* v3.2.0...v3.4.4

# Conflicts:
#	.template-lintrc.js
#	.travis.yml
#	README.md

* Run codemods

* Add some missing fa-icons and wrong translator

* Do not used reserved keywords in each loops (emberjs/ember.js#16826)

* v3.4.4...v3.6.1

* Run codemods

* Fix test and update yarn

Co-authored-by: Ruben Smit <RubenSmit@users.noreply.github.com>
@houfeng0923
Copy link

in ember 3.16.6, it still happen in ios safari.

@locks
Copy link
Contributor

locks commented Jul 22, 2023

I just went through all of the comments once again and I believe the linting rule and the current error message are sufficient to steer the developer in the right direction. Given that it also seems like fixing this issue might open up other wholes in the template compilation, I am closing the issue.

@locks locks closed this as completed Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests