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

Allow components to be named the same as void tags #1390

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Apr 27, 2022

Closes: #1388
Much thanks to @ef4 and @Windvis

tl;dr: <Link></Link> should be an allowed component invocation.

Why? ლ(ಥ Д ಥ )ლ

Background: embroider-build/embroider#1176
(though, @runspired isn't the first to run in to this, I've noticed this issue myself for the longest time, and just had a hard time understanding what was going on, because I was new into my "how do I even debug the things"-with-embroider-journey 😅

This is caused by this line.

Though, a bit of concern

The printer doesn't know about components.

I think long-term this is something that will fizzle out as a class of problem, because of strict mode (but who knows how long it'll be until everything is strict mode).

The drawbacks ┌༼▀̿ Ĺ̯▀̿༽┐

Anyone using Link, LINK, LiNk, lINK, liNK, linK as their style/asset loader in the their glimmer-parsed template will now not be a void element.

how many people are doing this tho? 🙃

The importance (ノ◕ヮ◕)ノ*:・゚✧

Not being able to have a <Link> component with embroider-using apps is a blocker for many many many many apps / addons I maintain from being able to adopt embroider.

ef4 and others added 2 commits April 27, 2022 18:23
…rd HTML tags

This includes a failing test case. If you parse to ast and immediately print back out, it turns:

```hbs
<Link></Link>
```

into

```hbs
<Link>
```

This behavior is special for `<Link>`. Other names don't do this.

I suspect it's because, in strict HTML spec terms `<Link>` is the same as `<link>`, and `<link>` is supposed to not have a closing tag. But in Glimmer/Ember terms, `<Link>` is a component and dropping the closing tag introduces a syntax error.

This was found in embroider-build/embroider#1176
@acorncom
Copy link
Member

Per embroider-build/embroider#1176 (comment) it looks like there might be some other cases that might be worth testing / handling here?

@NullVoxPopuli
Copy link
Contributor Author

Makes sense to me! Updates pushed!

@Alonski
Copy link

Alonski commented Nov 21, 2022

Hey,
Has this been released in a version of Ember by chance?

@chriskrycho
Copy link
Contributor

No, I'll chat with folks and see if we can get it into 4.8 as a bug fix (as well as the 4.9 beta releases).

@Alonski
Copy link

Alonski commented Nov 21, 2022

Is it possible to also hotfix this for older versions as well? Specifically 3.28?

@chriskrycho
Copy link
Contributor

Almost certainly not. This is not a new bug, and it's definitely not a security bug (3.28 is in the security-only phase of its LTS cycle). I understand that it's annoying, and we will definitely get it onto 4.8 and possibly onto 4.4, but we will need to discuss even the 4.4 case. It would be one thing if this were a regression, but it is not, as far as I can tell, and we don't make a habit out of doing significant backports for general bug fixes, else we would end up having to do it for basically every bug fix.

Don't get me wrong: I am sympathetic and it is annoying! But there are also costs to a backport: if we mess something up, we then have to revert or try to fix that, which causes yet more churn and difficulty for our users! And: there are only so many hours in the day.

@Alonski
Copy link

Alonski commented Nov 22, 2022

Yes makes sense. @bertdeblock do you think we can add a disclaimer for the lint rule or something? It's somewhat unusable right now no?

@bertdeblock
Copy link
Contributor

bertdeblock commented Nov 22, 2022

@Alonski To fix the lint rule, I think we just need a new release here and then update the @glimmer/* packages in ember-template-recast, specifically @glimmer/syntax. We could also bump ember-template-recast in ember-template-lint afterwards (+ release), to force the fix for end users. So, we don't need any new Ember releases to fix that AFAICT.

@Alonski
Copy link

Alonski commented Nov 22, 2022

Oh that makes sense. I was thinking we had to update Ember source as well but what you said is that we don't. Thanks!

@bertdeblock
Copy link
Contributor

Would it be possible for someone to already release this? Getting it in the appropriate Ember versions can still be done / discussed afterwards I believe? Having this available in a new release would already allow us to fix the issue over in ember-template-lint I think.

@chriskrycho
Copy link
Contributor

I pinged a couple folks with permissions in Discord but I believe one is on vacation and the other had a sick kid this week.

@ef4
Copy link
Contributor

ef4 commented Feb 10, 2023

Comment via @wycats: there may still be a remaining bug here because if the tokenizer is not scope-aware, not-capitalized cases could still get wrong handling:

<Thing as |link|>
  <link></link>
</Thing>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants