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

Doesn't work on components without a JS class in 3.13 #335

Open
knownasilya opened this issue Sep 27, 2019 · 25 comments · May be fixed by #339
Open

Doesn't work on components without a JS class in 3.13 #335

knownasilya opened this issue Sep 27, 2019 · 25 comments · May be fixed by #339

Comments

@knownasilya
Copy link

knownasilya commented Sep 27, 2019

#293 (comment)

Solution would be to auto generate an empty JS class for components that don't have one.
Screen Shot 2019-09-27 at 12 08 55 PM

@knownasilya knownasilya changed the title Doesn't work on components without a class in 3.13 Doesn't work on components without a JS class in 3.13 Sep 27, 2019
@webark
Copy link
Owner

webark commented Sep 27, 2019

😭

@webark
Copy link
Owner

webark commented Sep 27, 2019

umm.. yea. So to maintain backwards compatibility, we'd have to dump an empty component file in if there was non for each template. The difficulty there is that due to the lax resolver, components can be anywhere, so it ends up dumping components next to route templates.

@webark
Copy link
Owner

webark commented Sep 27, 2019

I just won't be able to look into this until next week :(

@webark
Copy link
Owner

webark commented Sep 27, 2019

And with the new version, we are requiring "template only" components to have to use the this.styleNamespace property in the templates to grab that string. So one would have to modify all template only components.

@webark
Copy link
Owner

webark commented Sep 27, 2019

https://github.com/emberjs/ember.js/blob/151d8c57da2929dc5ff20e336518d6ea2d83d992/packages/%40ember/engine/index.js#L528

it looks like it's still getting registered, I wonder if you could pull it out of that.

@webark
Copy link
Owner

webark commented Sep 27, 2019

@knownasilya might have a fix for this. We'll see if it suffices for the mean time.

@webark
Copy link
Owner

webark commented Sep 27, 2019

Shoot. Ok. well, have to spend a little more time later.

@lifeart
Copy link

lifeart commented Sep 29, 2019

@knownasilya is it possible to assign styleNamespace using template ast-transform?

<div class="{{styleNamespace}}"></div>

-> AST

<div class="app_components_my-component"></div>

or kinda.

@webark
Copy link
Owner

webark commented Sep 29, 2019

@lifeart yea. that parts easy, but there’s is no way to distinguish route templates and component templates, or if a component template is going to need that or not at build time.

@lifeart
Copy link

lifeart commented Sep 29, 2019

@webark during transform time we can figure out template type, based on file name & location.

@webark
Copy link
Owner

webark commented Sep 29, 2019

you could have a template that’s “app/posts/author-comment/template.hbs” and call that with {{posts/author-comment}}

so..

  1. so you can do something like “if has no route or component file”. But this would still pick up route templates that didn’t need a route.js or templates that get loaded into a different components.

  2. only support classic and the new co location that just came out. But this would leave existing pod apps still broken. It also would not account for template sharing.

neither of those are great options. I would also be much more error prone to try to add this to templates in addon’s due to how they are structured as well.

@JamesS-M
Copy link

JamesS-M commented Jan 6, 2020

How's this coming along? I'm running into this issue with my app.

@webark
Copy link
Owner

webark commented Jan 6, 2020

I think going the way we discussed in #342 where we have the option of wrapping all templates in a let block that defines a “styleNamespace” local variable is best for now.. That should be quick to do.

@JamesS-M
Copy link

JamesS-M commented Jan 6, 2020

@webark Just want to clarify. This is an update to the repo that you're talking about, rather than me wrapping all of the components that I'm working with in a {{let}} block?

@webark
Copy link
Owner

webark commented Jan 7, 2020

correct

@erikmellum
Copy link

Any updates on this? We also ran into it in our app.

@ctaylor-nurx
Copy link

Bump/+1: I'm stuck on ember 3.12 due to the break when I switch to 3.13.

@lifeart
Copy link

lifeart commented Oct 7, 2021

@erkie
Copy link

erkie commented Oct 7, 2021

We settled for a similar thing! After switching to that it's a lot nicer than leaning on an implicitly created div

<div class={{style-namespace-for "my/component"}} ...attributes>

</div>

or

<h1>Not this</h1>

<div class="{{style-namespace-for "my-component"}} more-classes" ...attributes>
   <p>It adds flexibility to where you can apply the namespace</p>
</div>

<div class={{style-namespace-for "my-component"}}>
  <p>You can also use it multiple times :D</p>
</div>

@webark
Copy link
Owner

webark commented Oct 7, 2021

yea. I am in the process of rewriting this (again) webark/ember-cli-styles#1 . I am very close (it's been about a 3 year journey) but here I'm doing something similar by using a element modifier.

There's a few discrepancies between the broccoli trees embroider provides and non embroider provides, so I'm trying to find a simple way of consuming both and having it work. Should be soon for a beta release of those packages.

@ctaylor-nurx
Copy link

oh nice! thanks!

@SergeAstapov
Copy link
Contributor

I am very close (it's been about a 3 year journey) but here I'm doing something similar by using a element modifier.

@webark will it work with FastBoot considering element modifiers do not run in FastBoot?

@webark
Copy link
Owner

webark commented Oct 7, 2021

@SergeAstapov well.. :sigh: well no then. I could adopt what I'm doing with the element modifiers into a helper. The ast will be more complex though.

bascially what I am doign now is taking an element modifier like

{{style-namespace '--variant'}}

and turning that into

{{style-namespace "--variant" buildClass="__modified_class_for_my-component" argsClass=@styleNamespace runClass=this.styleNamespace}}

with an ast (and only adding the buildClass, argsClass, and runClass, if they are not already set). Then, it takes basically does runClass || argsClass || buildClass and adds what you pass in on the end of it. This still gives you the flexibility of manually overriding the namespace, which is useful with modals or extending components (which now is frowned upon I believe, but wasn't always) or things like that.

You could modify that same behavior to work with a helper, the AST will just not be able to be as terse.

@webark
Copy link
Owner

webark commented Oct 7, 2021

@SergeAstapov I guess the AST is the same..

https://astexplorer.net/#/gist/3a94c003f5963bb4b18bf55a2584d38e/8c5f9652dafb0e472e58701e087b758b88ade0b4

So that can be an easy transition.

@webark
Copy link
Owner

webark commented Oct 7, 2021

updating the specs will be more annoying

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