webark / ember-cli-styles

4 stars 2 forks source link

Future of this addon #1

Closed jembezmamy closed 2 years ago

jembezmamy commented 3 years ago

Hey @webark, I've landed here from https://github.com/ebryn/ember-component-css/issues/355 so you probably know what I'm going to say. :)

I'm trying to migrate my app to Embroider and ember-component-css is currently the biggest obstacle. So it would be great if we could make this addon work and publish at least a basic version of it.

So my question is: how can I help? :) What needs to be done here to make this addon work with Glimmer and Embroider?

Also, I tried to install the most recent version and there are some issues with ember-cli-styles-colocation@* dependency, could you please fix that? I'm not sure what was the intention here. :) I tried symlinking https://github.com/webark/ember-cli-styles-collocation but without any luck.

webark commented 3 years ago

@jembezmamy if you were able to get a sample app setup with embroider and some styles, I'll get it working for you by next week.

jembezmamy commented 3 years ago

Wow, thanks, that would be great! I'll prepare the sample app and let you know.

hoIIer commented 3 years ago

thank you both, have enjoyed using ember-component-css and would like to migrate to this once it's ready so I can update my app, let me know if you need help testing or anything

webark commented 3 years ago

oh gosh. I just opened this up and my latest changes were not pushed.

webark commented 3 years ago

I pushed those, but it seems that the tests are not running. They were running before. I'll look into this and see what's happening.. It seems something screwy is going on.. (this is what happens when you come back after a year of not looking at it πŸ˜‚ )

webark commented 3 years ago

Ok. I pushed everything and the tests for "ember-cli-styles-namespace" are passing. So should be able to look there how to use this. I haven't published beta versions of these packages yet as an FYI

jembezmamy commented 3 years ago

@webark Ha ha, great, thanks. πŸ˜„ I'm going to work on the embroider demo next week, currently I'm on a short vacation.

webark commented 3 years ago

haha!! I'm on vacation too.. First "two night" vacation i've had with my wife since our daughter was born 6 YEARS ago!! I'm sorry that i've dragged this out.. I'm really interested in this working with embroider, and have some time in the next couple of months to spend on it. If someone can get an embroider app setup with this, i'll be able to make sure it's working.

hoIIer commented 3 years ago

I might take a stab at upgrading soon.. I'm on 3.20 still. Big thanks to you both for working on this.

Random question: how hard would it be to add option to minimize all classes e.g. "3824jmy-class--foo-123" -> "xy123"

webark commented 3 years ago

there's that option in the current iteration, I forget if I carried that over to this "namespace" method.

boris-petrov commented 3 years ago

@webark - in addition to adding Embroider support (which would be nice, of course) I believe a couple of more things are more important - namely Ember 4.0 support, tagless/template-only/Glimmer components support and co-location.

Embroider, even if released soon, won't replace ember-cli which will still be supported for probably years to come. However, Ember 4 is coming very soon and ember-component-css won't work on it because of some deprecation warnings. I believe that is the most important thing for ember-cli-styles to fix - so that this addon is usable on the latest Ember version.

Tagless components support would also be nice but there is a workaround for that right now - using stylesNamespace. So that could wait.

Co-location also would be very nice but that could also wait.

If you need example apps for any/all of these issues, just tell me and I'll prepare them immediately. :)

Thank you for taking the time to work on this! We love ember-component-css and would like to continue to use it going forward with the latest Ember versions. :)

webark commented 3 years ago

This rewrite handles these cases the best way I have found to satisfy tagless components. Currently it wraps each temple with a let block providing access to the styleNamespace property to be used in the template. This requires the developer to specifically add this class to those tagless components, but to keep to the same spirit of how this addon handles its notion of "namespacing" styles (i.e. .namespace SELECTOR) If there are other methods, i an open to explore those.

Co-location (as I understand it) is also supported. Single file components are not supported, but those are experimental, and support could be added later.

I have not attempted to use it in an embroider setup, but the new mechanism should carry over.

The main thing I need to work on to get this released, is the legacy support (which i'd be open to releasing these addons without that) and some real wold flexing and vetting of this solution. (which I have not done as I am no longer working on ember apps, or much in front end apps in general and am, sadly 😩 in react world when I do now).

If a subset of you all are open to helping me vet this new approach, I will commit to offering assistance and continued development as I can in my spare time. And if people start to contribute, give privileges of shared ownership of).

Does that answer your questions? If people are open to a zoom sometime, I'd be open to that as well.

boris-petrov commented 3 years ago

Thanks for the comment!

I believe that wrapping each template in a let block is fine...ish. Right now I'm using a helper which uses podNames from ember-component-css/pod-names and pass it the component's name. That is definitely not ideal but perhaps better in some way - as the template is not modified in any way by the plugin. Not sure though - perhaps the let approach is also OK.

As for legacy support - I think that could be skipped entirely given that you don't have all the time in the world. You can always release a new major version and tell people they should change this and that in order to continue to use the addon. It's totally fine to break backwards-compatibility (even greatly) in order to be able to support a package. The other way means we don't get anywhere because you're trying to implement/support features that you don't have the time for. So if you think the current state of affairs works - you can try releasing a beta, explain what is changed for users of the plugin, and wait for bug reports. :) It's easier for people to change a few things here and there on how they use the addon than a maintainer to support a million old things.

I didn't understand about Ember 4 support - are the deprecated things fixed and is Ember 4 supported?

webark commented 3 years ago

Yes to ember 4 support. It should be fully supported with this addon as long as the breaking changes are addressed.

webark commented 3 years ago

And ok. That's a good point about legacy support. I will tie up some loose ends and get a beta release out of these.

jembezmamy commented 3 years ago

@webark I've pulled the newest version of this repo and I still have hard time installing this locally (yarn install fails because of missing ember-cli-styles-colocation package, I suppose it's a git submodule but I can't pull this cause it's missing url or something). So maybe I'll wait for the beta release with further experiments. :D

webark commented 3 years ago

I recently fixed that. You should be able to just yarn link it.

webark commented 3 years ago

wait what? You are right.. I thought I just pushed that.

webark commented 3 years ago

OOOKKK... Now there we go. Sorry about that @jembezmamy

joelcox commented 3 years ago

What would be the best way to help/test this add-on? I don't have any practical experience with developing Ember addons, but sure could give it a try if that helps. What is the best way to test-drive this? Pull the Lerna repository and individually link all different packages into a project?

hoIIer commented 3 years ago

@boris-petrov "Tagless components support would also be nice but there is a workaround for that right now - using stylesNamespace. So that could wait."

Could you elaborate? The only reason I haven't moved to glimmer components is lack of support for tagless components.

boris-petrov commented 3 years ago

@burritoIand - well, you're missing on the best parts of Ember! :smile:

app/helpers/style-namespace.ts:

import Helper from '@ember/component/helper';
import podNames from 'ember-component-css/pod-names';

export default class StyleNamespace extends Helper {
  public compute([componentName]: [string]): string {
    return podNames[componentName];
  }
}

Use in any template-only/Glimmer component:

app/components/foo-bar.hbs:

<div class={{style-namespace "foo-bar"}}>
</div>

Note that the argument to style-namespace should match the component's path. That's the only downside. Apart from that (and the deprecated stuff used from Ember), all works beautifully.

hoIIer commented 3 years ago

@boris-petrov very nice! thank you, I'm going to try this out :D

hoIIer commented 3 years ago

@boris-petrov only wish Glimmer components had the component name available by default so you could just do something like {{get-style this}}__foo where the helper does podNames[component.name] (can use component.debugName but you have to manually set it in the component)

thanks again!

webark commented 3 years ago

Ok. I just rewrote the git history of this cause it was in a messed upstate. And cleaned up some loose edges. I have a couple of style things to get the lint passed for the tests, then I will release a v1 beta. I'll also add some basic usage so people can see how things have changed.

The next parts after that would be adding a migration plan (which will be minimal) and then finishing the addon for the legacy support (won't be terrible).

I know this is all way too late.. But better late than never. Sorry for the burden having an outdated package has been :/

boris-petrov commented 3 years ago

Absolutely no problem! Thank you for still taking the time to work on this! Also, Ember 4 is still not out so it’s definitely not too late. :) I’ll await the beta release and will try it out as I want to get rid of the deprecation warnings (and soon update to Ember 4).

webark commented 3 years ago

So it looks like embroider doesn't support normal pods, and only supports colocation. So I'm going to update the tests to that, then I should be able to release the beta.

webark commented 2 years ago

OK. Just pushed a change to switch from modifiers to helpers based on feedback from @SergeAstapov in which fastboot doesn't work with modifiers. (was brought up in https://github.com/ebryn/ember-component-css/issues/335)

The last things to do are...

webark commented 2 years ago

I also have never released something with lerna.. so if someone knows the command to trigger an alpha release, I will run that.

boris-petrov commented 2 years ago

@webark - a few comments:

1) as I mentioned above, Embroider support could come at a later point. No-one is still using it in production so that could wait a week or five. 2) You can drop support for Ember pre-3.24. People could use ember-component-css until they update to Ember 3.24 and then switch to ember-cli-styles. No need to burden yourself with supporting (very) old versions of stuff. 3) As far as I understand, ember-cli-styles will use some AST transformation so I can write {{style-namespace}} or something without giving it the component's name and that will be transformed by ember-cli at build-time, correct? But keep in mind, please, that people will also be using this helper in a "runtime" environment that doesn't get pre-processed by ember-cli - in that case I guess the syntax will be as is right now in ember-component-css - {{style-namespace "path/to/component-name"}}, right?

And again, thank you for the time!

P.S. oh, and about releasing - I use release-it. It's dead simple. You can try it out if you want.

webark commented 2 years ago

@boris-petrov Sure on the embroider stuff, I just am "in it now" and if I am nervous after I wrap this up I won't be for a while, and will deal with a lagging problem. But that's why a pre release could help.

Sure on the 24 stuff.. It appears to be small, but sure.

On the helper, you won't have to pass anything in. There is an ast that adds 3 arguments, the compiled class, and then a @styleNamespace and this.styleNamespace option. So you will just have the helper of {{style-namespace}} and nothing to pass in.

I can look at release it. Thanks.

webark commented 2 years ago

release it looks fancy! Maybe i'll try that out.

webark commented 2 years ago

@boris-petrov Im not sure I follow what you mean completely by

But keep in mind, please, that people will also be using this helper in a "runtime" environment that doesn't get pre-processed by ember-cli

The AST just adds the arguments, it doesn't like swap out the name. The helper still exists. There is no "master manifest" like in ember-component-css however (this was the cause of many bugs and issues), but you can now import a class name from the "style file". And this is how it does its route nampesacing and non glimmer component namespacing. (thought i guess glimmer tagless components are the new default now? πŸ€·β€β™‚οΈ)

boris-petrov commented 2 years ago

@webark - my question was more for non-compile-time templates. For example imagine that I have the template-compiler running in the browser. I can use it to compile templates at runtime. But they don't have a "file path" so there is no CSS file that matches that path. Is there a way to reuse one of the existing CSS files that have been bundled at compile-time?

The way this works right now: I have a file app/styles/components/some-component.scss. Using the style-namespace helper from above I can compile this template with the template-compiler (while the app is running!): <div class={{style-namespace "some-component"}}></div> and that would add the correct CSS class from some-component to this div.

My question was whether that would be possible using the new way of things.

webark commented 2 years ago

@boris-petrov you can pass in an argument to the component like

<MyComponent @styleNamespace={{style-namespace}} />

of in the component.js you could do a

import { styleNamespace } from <path to style file.extension>

and then set that to a class property, and if you set it to styleNamespace any helper useage will now use that instead.

I wrote this in the linked issue, but the ast basically makes the helper as if you had written

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

Does that solve what you mean? Or is there something else you are getting at I'm not following..

boris-petrov commented 2 years ago

I'm not exactly sure yet. :smile: I'll dig in this deeper once I can use a version of ember-cli-styles. So let's leave the discussion for now and I'll let you know if there is something missing as a feature after I've migrated. Thanks!

webark commented 2 years ago

@boris-petrov Do you have an example app/addon that you compiling the templates in the browser where you are concerned about how it will work I could take a look at to better understand?

boris-petrov commented 2 years ago

hm, that's not a bad idea - let me prepare one and I'll let you know. Some time today.

boris-petrov commented 2 years ago

@webark - https://github.com/boris-petrov/ember-component-css-example

Clone the repo, npm install, ember serve. The relevant code is in app/controllers/application.js:

getZone('<div class={{style-namespace "some-component"}}><a href="#">aaaaa</a></div>')

This generates, at runtime, the following element:

<div class="__some-component__45cbc"><a href="#">aaaaa</a></div>

And the correct CSS is applied.

Please ask if there is something you don't understand.

boris-petrov commented 2 years ago

@webark - I don't want to sound pushy but are there news on the beta release? :smiley: Ember 4, as I understand, is coming in a week or two so it would be nice to migrate from ember-component-css as soon as possible. Thank you! :)

webark commented 2 years ago

@boris-petrov ok. I published what I had so far.

here's an example.

https://github.com/webark/ember-cli-styles-example

right now you might have to add both

    "ember-cli-styles-colocation": "^1.0.0-alpha.1",
    "ember-cli-styles-namespace": "^1.0.0-alpha.4",

I haven't looked into your example yet. I will do that this week.

boris-petrov commented 2 years ago

@webark - thank you very much!

I believe you meant that both ember-cli-styles-namespace and ember-cli-styles-preprocessor must be added, because ember-cli-styles-colocation is a dependency of ember-cli-styles-namespace while ember-cli-styles-preprocessor isn't.

Here's what I did:

1) I moved app/styles/components to app/components so that styles are co-located.

2) I removed my own implementation of helpers/style-namespace.ts.

3) In app/styles/app.scss I replaced @import 'pod-styles'; with @import 'ember-styles.css';.

4) I removed ENV['ember-component-css'] = { classicStyleDir: 'components' }; from config/environment.js.

Issues I have:

1) Running ember serve lead to an SCSS error for starters - in my app.scss I'm using an SCSS variable that is defined in one of the component SCSS files that obviously previously was made available to SCSS via @import 'pod-styles'; but now isn't for some reason. Any ideas why?

2) Fixing this issue caused ember serve to start fine but going to the website reveals that CSS is missing and in the file where it should be there are 5 lines of /* Empty File. If know how to conditionaly run broccoli plugins, please reach out. Thanks. */, I guess coming from here. Is somehow SCSS not supported?

3) A major issue that I saw - the function add-component-style-namespace here looks-up all components on page-load (or at least the ones that have styles I guess?). This causes Ember to create an instance of this component and sends the init event - which causes observers on init to be triggered, init methods to be invoked and all horrible kinds of things happen because of that. This issue is a show-stopper. I'm not sure if Ember allows looking-up a component without it being instantiated.

Thanks again for your time! I'll appreciate any help on these issues.

webark commented 2 years ago

@boris-petrov you don't need the preprocessor.. you can use your own. You need both the collocation and the namespace cause they both have preprocessors that need to run, and the collocation one wasn't running unless O added it specifically.

webark commented 2 years ago

The "empty file" comment is from the preprocrssor. It creates an empty style file if it doesn't exists, wasn't sure how to handle it otherwise. If you know i'm all ears.

webark commented 2 years ago

A major issue that I saw - the function add-component-style-namespace here looks-up all components on page-load (or at least the ones that have styles I guess?). This causes Ember to create an instance of this component and sends the init event - which causes observers on init to be triggered, init methods to be invoked and all horrible kinds of things happen because of that. This issue is a show-stopper. I'm not sure if Ember allows looking-up a component without it being instantiated.

The need here is to auto add the namespace to the component. Do you know of a way that could be handled more efficiently?

webark commented 2 years ago

@boris-petrov I updated https://github.com/webark/ember-cli-styles-example with an example using scss variables.

webark commented 2 years ago

and @boris-petrov I don't have permission to your repo, but was able to achive the support by doing

diff --git a/app/controllers/application.js b/app/controllers/application.js
index d7ee272..292e76b 100644
--- a/app/controllers/application.js
+++ b/app/controllers/application.js
@@ -1,5 +1,6 @@
 import Controller from '@ember/controller';
 import { compileTemplate } from '@ember/template-compilation';
+import styleManifest from 'ember-component-css-example/components/some-component.css';

 function registerTemplateOnlyGlimmerComponent(moduleName, getTemplate) {
   define(moduleName, ['exports'], function (_exports) {
@@ -19,5 +20,5 @@ function getZone(template) {
 }

 export default class ApplicationController extends Controller {
-  zone = getZone('<div class={{style-namespace "some-component"}}><a href="#">aaaaa</a></div>')
+  zone = getZone(`<div class=${styleManifest.styleNamespace}><a href="#">aaaaa</a></div>`)
 }

and then moving the style file to app/components/some-component.css and adding

    "ember-cli-styles-colocation": "^1.0.0-alpha.1",
    "ember-cli-styles-namespace": "^1.0.0-alpha.4",
    "ember-cli-styles-preprocessor": "^1.0.0-alpha.2",

(ember-cli-styles-preprocessor) is just cause there isn't a css processor to handle the import.

not sure why I coudln't

import { styleNamespace } from 'ember-component-css-example/components/some-component.css'

I'll have to look into that.

boris-petrov commented 2 years ago

@webark - thanks for the help! I used @import 'ember-styles.css'; instead of @import 'ember-styles.scss'; in my app.scss and that's why issue number 1 appeared. Changing it, however, leads to another trouble:

File to import not found or unreadable
  β•·
1 β”‚ @import "my-app/components/some-component.scss";
  β”‚         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That's because I'm using ember-cli-postcss. I've added a commit on your example repo to showcase the issue. Just run ember serve and you'll see it. What are we to do with that?

P.S. I've added some test includePaths in the settings but to no avail.

webark commented 2 years ago

@boris-petrov so. It looks like I'll have to open up the ability to customize the manifest generation sooner than later.. changing https://github.com/webark/ember-cli-styles/blob/87b40db1d1d81fc99efe7704b74de61e4640cc72/packages/colocation/lib/colocate.js#L24-L29 to

 default: '@import "../../<file-path>";', 

made it able to find the file. I don't know if you can get that resolution path othersize.

boris-petrov commented 2 years ago

@webark - that does seem to make it compile, yes. That leads to another issue, however:

Top-level selectors may not contain the parent selector "&".
  β•·
1 β”‚ & {
  β”‚ ^^
  β•΅

There seems to be some change in handling of SCSS files because that wasn't the case with ember-component-css. Ideas about that? :) Note that this is again using ember-cli-postcss.