ember-component-css
ember-component-css copied to clipboard
webark
Support colocated components
Added support for colocating the styles.css files in the components directory with the template and js files i.e.
components./my-component.hbs components/my-component.js components/my-component.css
No tests have been written because I do not know how to do them properly in an addon. Need to read more but need to fix this first.
I have been away from this add-on since before octane dropped, and it changed a number of internals so some things no longer work as expected. The earliest I could look at this would be next week, and I will try to do so.
Does this allow switching to colocated components rather than pods? I'm definitely interested in getting this working!
I'm sorry everyone. Life and work has been crazy. I am sorry for letting this linger so long, and letting you all down. I know OSS is great, and hard at times, but this is a project I have taken on, and sorry for letting you all down.
This is something I will commit to getting done or providing guidance for. Expect to hear back from me soon.
This will let you move from pods to colocated components. We moved everything over.
However, it does not support Octane components, only classic. Not sure how to handle Octane components because they no longer have an outer tag to hang to css on.
@imagolive tagless and glimmer components should be supported by manually adding class={{this.styleNamespace}} to the markup. We've been doing that for tagless for awhile.
Great pickup on the octane components.
In that case, this PR has been working fine for collocated components.
spent some time over the weekend getting up to speed with the current state of things, and this PR. Need one more weekend to iron everything out. Will have this merged and altered if needed (or an additional commit or PR merged if needed) early next week.
Template only components have no solution with this iteration. The solution has been to add a "styleNamespace" local variable or helper which can be used to add the namespace where applicable. This is an approach I have been playing around with in the v1 "registry-way" version.
Components that to do not have a wrapping element can manually put in the "styleNamespace" property.
If we could get that test passed for at least the "unique component paths" test, I'd be fine merging it. I left a comment on what I think is the failing part.
Also, I spend some more time preparing for the 1.0 release, and I think we are all ready to go. It would be nice to get this working so that people can upgrade at their leisure as there are a few breaking changes.
@webark perhaps this is out of scope, but is there any planned support for glimmer components?
@rwwagner90 so for glimmer components, what I have so far, and I'm open to ideas, is what I'm doing is wrapping every template in a let block that adds a local "styleNamespace" variable which you can use. This is what the style file gets namespaced to as well. so you can use those in the components.
Since there is no wrapping element or even a component, that was the best though I had. It would be nice to do that though a helper but wasn't sure how to get a helper to read from where it's being used, so that's, why I went with the wrapping, let block approach.
If you have any suggestions, that would be awesome!
@webark glimmer components can have a JS component. They are not always template only. For template only I think your let block suggestion makes sense, but for ones with JS I would love to keep a similar API to normal with passing this.styleNamespace to class.
@rwwagner90 yea, this is what I'm doing..
https://github.com/ebryn/ember-component-css/blob/60f261f99797864ae5bd730d6d5799dc7a95cac0/lib/scope-templates.js#L15-L19
so it will still be there.
@webark ohhhhh, so you are saying the let is in the library, not a thing we would have to do in our apps? Sorry, I think I misunderstood. It might be helpful to have @chancancode and/or @pzuraq weigh in on how to best work with glimmer, colocated templates, etc in general.
@webark Does this mean that there will be different semantics for template-only glimmer components? i.e. it'll be styleNamespace rather than this.styleNamespace since there's no this without a backing class?
@adambedford yea. I would suggest just always using styleNamespace without the this. It will have to get added as a global for the template lint. If prefer to have it be consistent. I’m going to try the helper again and see if i can get it to work so that it will be consistent
@webark I wonder if rwjblue/ember-template-invocation-location could help in any way, maybe for inspiration or some tips&tricks, with Glimmer components support. Thank you for looking into this!
@SergeAstapov for the template stuff, this uses a template ast transform. I considered doing that. We could add a fake helper of style-namespace that just gets switched out at build. Doing a dynamic variable name for normal components is handy though cause there have been times where i have inherited that, or pushed it into another component. So giving an actual variable has value.
i guess we could have the ast do the let part and pass in the (this.styleNamespace or "styleNamespace") .. hmm.. that's actually not a terrible idea. I might look into.
might give us the best of both worlds. Handlebars AST's are annoying though.. I might work though that. you'd have to catch the different uses for it.. like it it was passed to another component, or if it was concated, or in a hash helper or whatever
@webark - wrapping every glimmer component in a let block sounds bad for performance I guess? Why not stay only with the helper? That's what I'm currently doing (of course, now I have to manually pass the template's location which must be "fixed"). I can't think of a use-case where the helper wouldn't be enough - even if someone needs a local variable, they could always do a {{#let (style-namespace) as |styleNamespace|}} and use that, right? I would be strongly against wrapping every single component with something that won't be used in 99% of the cases.
@boris-petrov i'm not sure it would be a performance hit, but i agree with the sentiment. It's not ideal, but it's predictable.. I wish every there was an actual story around a finalized identifier for every item (template, component, route, controller, etc) that was part of a public api. I think it would open the door to more possibilities. Even the prevalence of the the use of "_debugContainerKey" in addons (even official addons) implies it's something that would be valuable and is needed.
having a transform of style-namespace to (style-namespace (if this.styleNamespace this.styleNamespace (if this.args.styleNamespace this.args.styleNamespace ${atBuildStyleNamespace})) has its downsides as well.. But i think it could be better.
I like having the this.styleNamespace being a public api, cause it opens up some useful patterns. But then you'll have some some instances in a code base of style-namespace and others of this.styleNamespace or which isn't great either.
But it's all about trade offs I guess.
@webark - thanks for the discussion and trying to find the best way!
I'm not exactly sure what you want to do with the style-namespace helper but wouldn't it be some compile/runtime thing that will just do something similar to this:
export default class extends Helper {
compute([componentName]) {
return podNames[componentName];
}
}
Just without requiring componentName to be passed?
If that is so, why is there a need for anything else? Can you give me some example where that wouldn't be enough?
No worries! This issue is the main reason this transition has taken so long with not much movement in the codebase. I don't know the best way to do this, so it's helpful to talk it over with someone!! So thanks for discussing!
so, there no "podNames" in the new way. Each style file gets a corresponding "style.js" file that has a "styleNamespace" property on it. the same thing could apply though and you could do an "owner.lookup" with the path to get it's corresponding style file.
But at least in my usage, there are a non insignificant amount of times where i pass in the styleNamespace is n some instances to modify the set of styles that are applied, and would want to accommodate those cases as well.
OK, I'm not familiar with the new way, but are these modifications that you do done in the JavaScript component file or in the template somehow? Because I'm talking only about the template "API" that people will use (that a single style-namespace helper shoild be enough). In the JS they can do anything they want (and ember-component-css could expose all kinds of JS APIs for querying/modifying the CSS I guess). Not sure if what I say makes sense when we're talking about this new implementation though.
@boris-petrov another annoying thing is that the ember parser expects templates to have arguments (which ends up getting added), but the lint rule fails cause it doesn't like a helper with no arguments... :(
https://github.com/emberjs/rfcs/pull/353
yea, that hasn't been merged in yet though.. So.. hmm.. :/
@webark there is a new API that was introduced for building the component tree in ember-inspector which might be helpful for finding all the components. We are using it here https://github.com/emberjs/ember-inspector/blob/7f51cab365a275269951057e3ae7ee0278898b77/ember_debug/libs/capture-render-tree.js#L12
I believe it is not on in production by default though and could cause performance issues. If there is something else private that works well, we could talk to Ember Core about making it public.
@webark - element modifiers have been implemented and working in Ember for a while now. Not sure why this RFC is not merged.
https://astexplorer.net/#/gist/0366b1f81f1b6c2ae7e086e4709fed4c/latest
This is what is looking promising.
The three parameter names will probably change, but I feel this is the best approach I have yet. It will take an amount of nontrivial migration. :/ But it seems like it is the most sustainable solution.
so this is kind of what the syntax would be.
{{#each this.items as |item|}}
<span {{style-namespace "--vairant"}}></span>
<span {{style-namespace (concat "__element--" item)}}></span>
<span {{style-namespace "--vairant" namespace=this.newStyleNamespace}}></span>
<span {{style-namespace namespace=this.newStyleNamespace}}></span>
<span {{style-namespace}}></span>
{{/each}}
I'll have to check what setting this on if you can use modifiers on classic invocation style. I don't think so though. At that point you would have to just use the this.styleNamespace I think
so this is kind of what the syntax would be.
{{#each this.items as |item|}} <span {{style-namespace "--vairant"}}></span> <span {{style-namespace (concat "__element--" item)}}></span> <span {{style-namespace "--vairant" namespace=this.newStyleNamespace}}></span> <span {{style-namespace namespace=this.newStyleNamespace}}></span> <span {{style-namespace}}></span> {{/each}}
Is vairant correct here or should that be variant?
Yes! That was just an example of a string that gets added to the end of the class.. so special meaning
Gotcha, so to be clear the new API would be to use this element modifier in place of this.styleNamespace? Will this work when styles.scss lives in pods or colocated component folders?
@rwwagner90 yea, we got a little off-topic here. It was just where the conversation was happening so I carried it on. But this would be in addition to for mainly template only components.
The co-location stuff I was going to work through next. I already have some preliminary work done on it, but was finishing that up. I'm deciding how much stuff to backport, or to write a legacy bridge, or to modify stuff as is.
if writing a legacy bridge doesn't take too long, I'll pursue that. If it starts to get hairy, I'll just backport the element modifier and co-location work and have this version be octane supportable as well.
@rwwagner90 yea. Trying to find the best way to get the namespaces into the handlebars ast. Have a couple of ideas (like adding a temp variable with the ast that gets swapped out later in the post process) but don't see a strait forward way.
ohh.. there's a "meta.moduleName" https://github.com/rwjblue/ember-template-invocation-location/blob/master/lib/ast-transform.js#L207 nice.
so where I am out now.. is currently I am saving a file
return `
import EmberObject from '@ember/object';
export const styleNamespace = "${styleNamespace}"
export default class extends EmberObject { styleNamespace = styleNamespace };
`;
as a style.js file at the same location as the style file. This allows me to look up the file by doing
owner.lookup(`style:${stylePath}`)
The issue with this approach is that with colocation, I can't create a style.js file in the same location as the style file. What I would like to do instead of creating a style.js file, is just to take the existing style file and add js to it... so like style.scss.js or index.css.js or my-component.less.js.
However, I want to maintain the same lookup of owner.lookup('component:${stylePath}') to be analogous to owner.lookup('style:${stylePath}'). So I need a way to register a my-component.less.js file as a "style" namespace based off of the file ending of (less|css|scss|sass|style).js.
If I can find a way to register that resolver, then I don't have to do something where during build I look to see if it's an index.css and if there is a corresponding index.(js|hbs) and if the file path has /components/ in it to replace that with /styles/ .. which feels much more brittle.
@webark there are two types of colocation. One is "pod" like where you have a folder for the component with an index.hbs and index.js and another where there is no folder and foo.hbs and foo.js just live in the root component folder. For the nested case, I think looking up paths still works, for the non-nested case, perhaps we treat it like classic, non-pod components?
yea. But since you have an index js and .hbs, the logical thing would be to write a "index.css", but then we can't do another index.js, and will have to name it different. I like the option of importing the "index.css" there, and right now it has the namespace, but could be added with other info. it feels like the best pattern.
i looked into it a little bit, and think i have a way to grab them with the resolver.
@rwwagner90 ok. Added some actual co-location tests, and this is where I am at, which I still don't love this solution.
https://github.com/ebryn/ember-component-css/blob/1b7eea4afa31a7614913ff61cd2a81d8c4872c30/addon/utils/process-style-type.js#L1-L33
and I realized I had forgotten to push the modifier work. So that is not up.
Everything appears to be working now. There are some backward-incompatible changes that I wanted to add an extra optional option to support. But for forward-thinking individuals, who don't mind a small number of renaming somethings, this will suffice.
The breaking changes are listed at the top of #333
I will be releasing a beta soon, and then working on the new documentation before the final 1.0 release
@webark can you clarify what exactly is working now? Does this include colocated components both nested and not? Does it include a way to work with glimmer components, etc? Once I have clarity into which use cases this supports and what changes I need to make, I am happy to give it a try!
@rwwagner90 yep to all! I have a nested test, flat test, and pod test. Also template only and component. I haven't added a "glimmer" component, but either the modifier or the "this.styleNamespace" will work.
and @rwwagner90 if you or anyone else here wouldn't mind helping formulate a "roll out" plan, that would be appreciated. I was going to write one up in #333 later today or tomorrow. Any comments on that would be appreciated. I'll post a link to that once I have it.
it also looks like co-location breaks in 3.12, cause it tries to create a js file from the template, and the name already exists. https://travis-ci.org/github/ebryn/ember-component-css/jobs/711194656
@webark yeah, we should probably only do the co-location bits if Ember >= 3.13. Either that or don't test 3.12 anymore. Unsure of what is needed for a roll out plan. I would do a major version bump if we're dropping support for < 3.13 and then document to use the old version for 3.12 or below, and the new version for latest.
I think now the lowest common denominator is ember-modifiers. I'm just not sure how to conditionally include the collocated component test.
Well, i want an initial beta, then need to re add back in the various configurations you could have chosen, then a potential backwards compatibility addon, and then splitting up the addon into 3 separate ones and that including them here.
@webark I left a comment here https://github.com/ebryn/ember-component-css/pull/333/files#r460402288 I am not sure if this will work at build time or not, but assuming it will, this would check the Ember version before doing colocated stuff. It uses https://github.com/pzuraq/ember-compatibility-helpers. Maybe give that a try?
yea. The issue isn't with this addon, it's when ember translates the template. It tries to keep the same name and just change the extension.
ok. an "alpha" tag has been published with the version of 1.0.0-alpha.2
https://www.npmjs.com/package/ember-component-css/v/1.0.0-alpha.2
WIll create a new issue with the remaining work to get to a stable 1.0 version soon.
yea. The issue isn't with this addon, it's when ember translates the template. It tries to keep the same name and just change the extension.
Can you say more here? I thought the issue was we wanted to not run colocation stuff if on 3.12 or below?
in the tests dummy app, there is a co located test for an acceptance test. This fails to build on ember 3.12.
there isn't anything logically different it does in order to account for a co located style file.
https://github.com/ebryn/ember-component-css/blob/00b8ba3468754c6978c1e8ba3d0b0c3fc5b4d234/addon/utils/process-style-type.js#L13
and
https://github.com/ebryn/ember-component-css/blob/00b8ba3468754c6978c1e8ba3d0b0c3fc5b4d234/lib/component-names.js#L11
are the only things, but those encompass all.
@webark sorry for the delay in responding, but yeah we should wrap that test in ember-compatibility-helpers.
@webark where are we on this? Would be good to wrap this up and get new docs, so folks can use this addon still.