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

Future of this addon #1

Closed
jembezmamy opened this issue Jul 13, 2021 · 94 comments
Closed

Future of this addon #1

jembezmamy opened this issue Jul 13, 2021 · 94 comments

Comments

@jembezmamy
Copy link

Hey @webark, I've landed here from webark/ember-component-css#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
Copy link
Owner

webark commented Jul 15, 2021

@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
Copy link
Author

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

@erichaus
Copy link

erichaus commented Jul 22, 2021

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
Copy link
Owner

webark commented Jul 27, 2021

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

@webark
Copy link
Owner

webark commented Jul 27, 2021

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
Copy link
Owner

webark commented Jul 27, 2021

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
Copy link
Author

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

@webark
Copy link
Owner

webark commented Jul 29, 2021

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.

@erichaus
Copy link

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. "__3824j__my-class--foo-123" -> "xy123"

@webark
Copy link
Owner

webark commented Jul 29, 2021

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

@boris-petrov
Copy link
Contributor

@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
Copy link
Owner

webark commented Aug 3, 2021

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
Copy link
Contributor

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
Copy link
Owner

webark commented Aug 3, 2021

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

@webark
Copy link
Owner

webark commented Aug 3, 2021

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
Copy link
Author

@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
Copy link
Owner

webark commented Aug 7, 2021

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

@webark
Copy link
Owner

webark commented Aug 7, 2021

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

@webark
Copy link
Owner

webark commented Aug 7, 2021

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

@joelcox
Copy link

joelcox commented Aug 24, 2021

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?

@erichaus
Copy link

erichaus commented Aug 26, 2021

@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
Copy link
Contributor

boris-petrov commented Aug 26, 2021

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

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.

@erichaus
Copy link

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

@erichaus
Copy link

@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
Copy link
Owner

webark commented Oct 2, 2021

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
Copy link
Contributor

boris-petrov commented Oct 2, 2021 via email

@webark
Copy link
Owner

webark commented Oct 4, 2021

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
Copy link
Owner

webark commented Oct 11, 2021

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 webark/ember-component-css#335)

The last things to do are...

  • Embroider support: (they pass in a different broccoli tree to the css preprocessor which doesn't include js files. we handle this in addons, but non embroider doesn't include them for addons an apps.)
  • Pre 3.24 is having issues with the ast and the argument that I'm adding to the helper. When I manually add it using the same syntax it works, so I'm a little stumped on this one, but will circle back around to it after the emborider fix
  • Adding in the basic documentation on how to use the new helper and handle the other updates.

@webark
Copy link
Owner

webark commented Oct 11, 2021

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

@webark
Copy link
Owner

webark commented Nov 16, 2021

@boris-petrov for # 2 yes, this is a "compile" as in it is run during the preprocess step, rather than a "filter" which is just run during the postprocess step.

for # 1 .... 😔 The thing is.. manners by which this can be done "automatically" from my knowledge have all closed. I can look back and see if for an EmberComponent (which they say will be around for all of ember 4.0) the option of "reopening" is still on the table. Moving forward, I guess glimmer components are the defacto solution, so we shouldn't expect components to be given wrapping elements anyway, and then the need to use the helper will be forced. Perhaps the ember component part could be moved into a legacy support addon.

🤔

@webark
Copy link
Owner

webark commented Nov 16, 2021

maybe you can inject a computed property into components that would be able to do it.. 🤔

@boris-petrov
Copy link
Contributor

boris-petrov commented Nov 16, 2021

@webark - so for issue number 2 - is there a way to also add a filter step? As I currently have such in my ember-cli-postcss config and would like to migrate that as well.

For number 1 - the thing you do in that code is to set the correct style file to the component? If there is no "automatic" way - why don't you revert to "manual"? That works totally fine right now (even though it's verbose to pass a string with the same component path as an argument). Also, you could make it accept both a string and a "real" style like in the example you gave. In any case that's much better as the way it is right now is unusable. 😢

@webark
Copy link
Owner

webark commented Nov 16, 2021

@boris-petrov yea. I think i will add another addon, something like "ember-component-auto-namespace" that will handle this legacy behavior, and then the standard behavior will be the manual process of the helper, or importing the namespace and adding it the the component class.

@boris-petrov
Copy link
Contributor

You mean the ember-component-auto-namespace addon will handle the NEW behavior? And the default will be the legacy way - manually specifying a string or the imported namespace? Or did I misunderstand you?

@webark
Copy link
Owner

webark commented Nov 16, 2021

no, that would handle the old way of adding the styleNamespace to the "classic componet" (i.e. ember component).

@boris-petrov
Copy link
Contributor

Oh, I understand, you mean that the current way of automatically finding all components will be gone and only a manual way will be available (plus this new addon that could automate things for classic components)?

@webark
Copy link
Owner

webark commented Nov 16, 2021

and even though "Ember Components" are not going away in 4.0... it looks like both injections are off the table https://deprecations.emberjs.com/v3.x/#toc_implicit-injections and reopening https://deprecations.emberjs.com/v3.x/#toc_ember-built-in-components-reopen are off the table (like I had thought). So there is no way to handle this, as the way of looking up all classic components is not viable.. :( At least there is no way that I can see to accomplish this.

@webark
Copy link
Owner

webark commented Nov 16, 2021

only a manual way will be available

Correct. And anyone migrating an application will need to go through and add the import and assignment to each and every component class in their application and addons. :(

@webark
Copy link
Owner

webark commented Nov 16, 2021

Well.. I'll remove that bit of functionality sometime in the next couple of days, and release a new package.

@boris-petrov
Copy link
Contributor

Well, for me at least that's totally fine because anyway I'm doing the same right now for Glimmer components (which are the majority of components I have) with ember-component-css. I think that's the right way to go.

Thanks! After the release I'll try it out and let you know how it goes. :)

@webark
Copy link
Owner

webark commented Nov 16, 2021

back before i had landed on the lookup behavior, i was planning on just issuing a code mod. I had written up the babel ast. Maybe If i have the time I could dust that off and put it in a proper ember code mod... Big emphasis on "IF" i have the time.

@boris-petrov
Copy link
Contributor

That would be nice but it's not a blocker for users - just more painful. :) I would prefer for all the blockers to be fixed first and then the nice-to-have things to (eventually) follow so that we can upgrade.

@boris-petrov
Copy link
Contributor

@webark - any luck removing the auto-discovery functionality and enabling the "manual" version? :)

@boris-petrov
Copy link
Contributor

@webark - I just remembered that because there won't be an auto-discovery mechanism for old classic components as in ember-component-css, then there should be a way to manually set the class like before:

import { classNames } from '@ember-decorators/component';
import Component from '@ember/component';
import podNames from 'ember-component-css/pod-names';

@classNames(podNames['some-component'])
class SomeComponent extends Component {

@webark
Copy link
Owner

webark commented Dec 19, 2021

@boris-petrov I pushed the removal of the auto-discovery and also added an example in my example repo for your above example
https://github.com/webark/ember-cli-styles-example/blob/c8fbaec325f6c566037578eee6218e80bf2bc62c/app/components/test-classic.js#L1-L8

import { classNames } from '@ember-decorators/component';
import Component from '@ember/component';
import { styleNamespace } from './test-two.scss';

@classNames(styleNamespace)
export default class SomeComponent extends Component {

}

@boris-petrov
Copy link
Contributor

boris-petrov commented Dec 20, 2021

@webark - thank you again!

I tried out the newest versions and all seems to work great! I think we're almost to the point where I could use this instead of ember-component-css! :) Just a few (minor) things left:

  1. I have a number of cases where I want to pass to style-namespace an argument with a style's path but that to be dynamic and not imported from JS. That is, it's right now possible to do:
import Component from '@glimmer/component';
import { styleNamespace } from './test-two.scss';

class Foo extends Component {
  foo = styleNamespace;
}
<div class={{style-namespace runClass=this.foo}}></div>

And this works fine. However, I would like to be able to directly do:

<div class={{style-namespace runClass="test-two"}}></div>

Or something similar. Because, as I said, the parameter to runClass could be dynamic - coming from user input for example - not a fixed string.

I tried argsClass and buildClass too, not sure what they are for, but it doesn't seem to work. Am I missing something or is this not possible right now?

  1. Same thing as number 1) but to be used in JS. I pretty much need something like import podNames from 'ember-component-css/pod-names';. Is it possible to expose that?

  2. How do I add a filter step to the workaround you posted for ember-cli-postcss's issue? You mentioned that what you gave as an example maps to the compile step.

@webark
Copy link
Owner

webark commented Dec 20, 2021

@boris-petrov you would just have to import the namespace from wherever you want to use it. The CSS get scoped (as it does with ecc) during build time. So passing in a random string won't effect the CSS that has already been generated.

Do you have an example of how you are currently doing this with ecc and I can try to replicate that example?

@boris-petrov
Copy link
Contributor

boris-petrov commented Dec 20, 2021

@webark - sure, here you go. Only look at the third commit. This is the relevant code:

<Input @value={{this.foobar}} />

<div class={{style-namespace this.foobar}}>
  <a>Text</a>
</div> 

If you write some-component in the input field, you'll see the text turn red. Any other value will not make it red. That's because there is a some-component.css file which is applied when this.namespace == some-component. Is that possible with ember-cli-styles?

@webark
Copy link
Owner

webark commented Dec 20, 2021

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

Basically, you just have to import the styleNamespace from the style file you want to use the namespace of and apply it where you want to apply it.

@boris-petrov
Copy link
Contributor

@webark - sorry, I shouldn't have used namespace as a variable name, it's confusing. I force-pushed the commit (and edited my previous comment) - I've now used foobar.

You've misunderstood my intention. The idea is that I want to apply style-namespace with DIFFERENT values - here depending on user-input. If the user types "some-component" in the input field - some CSS is going to be applied. If the user types "other-component" - different CSS is going to be applied.

Run the app, type some-component in the input and see how the text below changes. Then, type other-component - you'll see different CSS applied. Sorry, maybe I sound a bit confusing, I'm not sure how to better explain. :)

@webark
Copy link
Owner

webark commented Dec 20, 2021

hmm.. I'll look at it another time. You'll either need to grab the list by generating it yourself through an import manifest of options, or you can look at the "hack" I did for route styles

return STYLE_EXETNSTIONS.reduce(function (allStyles, extention) {
return allStyles.concat(
owner
.lookup('container-debug-adapter:main')
.catalogEntriesByType(extention)
);
}, [])
.filter((value, index, self) => self.indexOf(value) === index)
.filter((stylePath) => styleFileExtentionRegEx.test(stylePath))
.map((stylePath) => {

@webark
Copy link
Owner

webark commented Dec 20, 2021

there can be no concept of the pod-names though. This is one of the main causes for needing to rewrite a lot of this addon. We can no longer reliably generate an aggregated list of these throughout all of the app styles and addon styles, and with embroider will be impossible.

You can generate one yourself through a self-managed import manifest, or you have to use the 'container-debug-adapter:main' which I'm unsure what that support will be long-term.

@boris-petrov
Copy link
Contributor

@webark - any luck figuring out how to do what I requested above? To help a bit - do you have any ideas how ember-css-modules works? It does something similar to ember-cli-styles and I believe it does have some concept like pod-names. Perhaps you can take some inspiration from there? :)

@webark
Copy link
Owner

webark commented Jan 13, 2022

@boris-petrov from my understanding at least, packages need to be static, and can not/should not be dynamically modified by other packages. I am not familiar with how ECM works. Do you have a coded link that you could provide?

From my understanding, all your concerns have been addressed, and you will need to maintain your own import manifest of possible components from which you want to dynamically generate and pull class names from.

basically, import all import { styleNamespace } from './test-two.scss'; from everywhere in your application you want, and then export that in a place you can look up.

@webark
Copy link
Owner

webark commented Jan 13, 2022

I'm going to close this issue as its primary purpose has been resolved. If you want to open a new issue specifically around a generated manifest that the addon provides, we can pursue that in a new issue. If you have any documentation or code references for that in ECM, that would be appreciated.

Thanks for working through all the issues we did on this one! @boris-petrov your beta testing has been extremely appreciated.

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

No branches or pull requests

5 participants