Updated to maintain roughly the same api, but remove monkeypatches #123

webark · 2016-03-09T02:14:13Z

and with minimum useage of private apis.

Also enables source mapping and hot realoding,
and sets us up for supporting in addon usage.

Need to add back in the accptance test after removed
it due to such severe ember upgrade differences.
Need to add support for plain css useage
Need to better differentiate between pod-styles.scss, and pod-styles.sass,
rather then just not accepting sass files and just scss styles instead.
Need better testing around empty states and precompilers other then sass

but with minimum useage of private apis. Also enables source mapping and hot realoding, and sets us up for supporting in addon usage. * Need to add back in the accptance test after removed it due to such severe ember upgrade differences. * Need to add support for plain css useage * Need to better differentiate between pod-styles.scss, and pod-styles.sass, rather then just not accepting sass files and just scss styles instead. * Need better testing around empty states and precompilers other then sass

webark · 2016-03-09T02:17:35Z

This will probably change a bit, but wanted to get this convo started. Would like to add back in some tests, but will hopefully fix #122, #121, #109, #96, #95, #82, #67, and others.

webark · 2016-03-09T02:20:13Z

And since I know that you have been concerned with the performance with this addon @stefanpenner, I would appreciate any feedback or suggestions that you have as well.

stefanpenner · 2016-03-09T05:05:14Z

addon/initializers/component-styles.js

+  });
+
+  Component.reopen({
+    addPodStyleName: Ember.on('init', function() {


use init + super instead, better performance.

topaxi · 2016-03-09T07:08:07Z

This looks very good!

To support plain css and fix #107 you could use the removed postcss-selector-namespace plugin together with postcss-scss and postcss-less. Although I only tested postcss-scss yet.

webark · 2016-03-09T08:00:36Z

@topaxi totally! I was wanting to avoid having to add in that overhead, and with plain CSS, you couldn't do the standard import to include it in the app CSS either. so would have to either tack it only to the app CSS or the vendor CSS manually, or include in a third CSS file with utilizing the headerFor hook. which might not be a bad idea, and could potentially be a better solution for engines long term. Would want others who weigh in who are more familiar and see if it would actually have an impact. Plain CSS is something that should be addressed either before this PR would get merged, or very soon after.

webark · 2016-03-09T08:47:05Z

actually, scratch that. I don't think that it would have any impact on an engine, you'd want to have a single file. And for preprocessed styles, the file needs to be able to be imported into an existing app stylesheet to be processed so that you can utilize variables, methods, and the likes. So, we should just tac the plain css onto the end of the app css.

webark · 2016-03-09T19:05:39Z

@topaxi on further thought, to try to do plain css without a preprocessor, and just doing that ourselves, would be too hacky and would not want to maintain. We will however be basically giving them a preprocessor, which feels weird, but necessary. What do you feel about using cssnext rather then sass or less..?

…penner

…penner suggestion

@stefanpenner

…eloads for better preformance per @stefanpenner suggestion

topaxi · 2016-03-09T19:40:32Z

Yes, there's definitely need for a compiler, I'd use postcss (cssnext is only a postcss plugin collection) with as few plugins as possible, and let the addon consumer opt-in for more plugins like cssnext. As I already mentioned, the postcss-selector-namespace plugin I wrote could prefix plain css with the component class, though I'm not sure if we can reuse this 100%. We'd probably also need postcss-import-url to handle the @import statements. If we could postcss all cases, and even run postcss after less and sass, that would probably the best way, but I'm not sure if we could do this.

webark · 2016-03-09T20:02:40Z

@topaxi Why do you want to run it for every case?

@stefanpenner

…d could potentialy be a computed property per per @stefanpenner suggestion

topaxi · 2016-03-10T07:04:32Z

@webark If we run postcss in every case, we can support every precompiler for css there is, so we can transform the output of all the precompilers instead of the original files, where we'd need to support less, sass, scss, stylus, etc. But as I said, I'm not sure if we can actually do this (my broccoli/ember-cli knowledge is a bit limited here).

webark · 2016-03-10T15:39:08Z

So every preprocsors that uses a unique extension that is not .css, supports nesting with the & selector and uses curly brackets should be supported. This leaves us at the .css files not being supported cause they do not support nesting with the & selector. I had previously assumed this would be only plain CSS files, but that's wrong cause these could be cssnext files also as they use the .css extention, but run a preprocessor on them. We still want the file to be prepped but not processed until after its imported, cause that way you can utilize global variables and helper methods and the likes that are defined in the styles directory in your component CSS files.

I think using that plugin for postcss you wrote to process the .css file would work well here for plain CSS files, and should be lighter weight then brining in sass or something like that. But as long as we keep the same API of the & selector and not let the user customize what that is, I dont see a need to use it for other files. But I totally admit that I could just be missing the point of what your after!!

topaxi · 2016-03-10T15:46:41Z

I see your point with & being an issue, I always used :--component even when using scss and see now how this won't work with being backwards compatible.

I think the best way forward and being compatible is only using postcss for plain css for now :)

If you're sure that just wrapping with .component-class { /* nested styles */ } is enough I think we're good. (I know there was an issue with this in #107 but this might already be fixed?).

topaxi · 2016-03-10T15:52:03Z

On another note, it seems your changes don't support sass and stylus indentation style, or am I missing something?

You'd probably have to do something along the lines like here: lib/broc-component-css-preprocessor.js#L37

webark · 2016-03-10T15:53:54Z

I removed sass as an option cause it's kind of deprecated, it would have to use a different output cause it doesn't support curly brackets, and then when importing to the scss file I had, I would have had to import pod-styles.scss rather then just pod-styles, which was annoying.

webark · 2016-03-10T15:55:09Z

And I thought stylus supported curly brackets... grr..

topaxi · 2016-03-10T15:56:18Z

Stylus supports curly brackets, though I'm not sure if mixing the two styles work.

webark · 2016-03-10T15:57:39Z

I'll have to look more into it. I didn't do any testing other then scss files.

webark · 2016-03-10T15:59:39Z

You could add a couple of new filters that handle the individual other file extensions. If a type used the same extension but different syntax that would be annoying.

webark · 2016-03-10T16:02:16Z

@topaxi what do you feel about allowing the defining of what the self selector is..? I kind of liked your approach of using :--component rather then the &

topaxi · 2016-03-10T16:06:04Z

Stylus definitely supports this:

body
  background: red;

  .someClass {
    background: blue;
  }

Wrapping in curlies doesn't seem to work though:

body {
  background: red;

  // Does not work
  .someClass
    background: blue;
}

I'd definitely prefer :--component all the way, it is more explicit than &. By allowing & we kind of introduce a second meaning to &.
What do you think @ebryn?

webark · 2016-03-10T16:13:37Z

I think I'm starting to see more of what you are talking about. And now's a good time to do a breaking change if need be.

So your saying, that we would open up every file, and then loop over each rule definition and either prepend a ".CLASS " or replace :--component with ".CLASS" and that way we don't need to worry about each preprocessors own idiosyncrasies..

webark · 2016-03-10T16:17:07Z

Id say the main downside for me is the extra more intensive processing step that requires. But the simplicity and uniform approach to all preprocessors is nice.

webark · 2016-03-10T16:18:43Z

if you did a rule that was just an & though, if you pretended that with a ".CLASS " it would still work. Right?

webark · 2016-03-10T16:19:54Z

And we couldn't prepend any rules that where nested. Does that plugin you wrote account for that @topaxi?

stefanpenner · 2016-03-18T04:58:40Z

index.js

+  _concatenatedPodStyles(wrappedStyles) {
+    var concatenatedStyles = [];
+
+    for (var i = 0; i < this.allowedStyleExtentions.length; i++) {


how many extensions are typical?

three are about 5 css preprocessing extensions? .scss, .sass, .less, . styl, and maybe one more?

webark · 2016-03-18T05:23:53Z

@stefanpenner thanks for reviewing this! I'll start working on those changes tomorrow while I'm filling out some tests. Thanks again!

…they will get included iwth the package

@stefanpenner

…rsistentOutput per @stefanpenner suggestion

webark · 2016-03-18T23:40:05Z

@stefanpenner I moved the generating of the pod-names.js file that gets imported into the initializer to use persistentOutput and fs-tree-diff. Let me know if that's what you where thinking for that one. And I'll either find a better solution for the removing of the files empty files with the other one, or switch it to use the same methodology.

rwjblue · 2016-03-21T12:34:04Z

addon/initializers/component-styles.js

+
+  ComponentLookup.reopen({
+    componentFor(name, owner) {
+      if (podNames[name] && !owner.application.hasRegistration('component:' + name)) {


On Ember 2.3 and higher, owner.application should not be used. You should use owner.hasRegistration and owner.register. To enable usage of the same API across older Ember versions you can use the ember-getowner-polyfill.

Also, wouldn't it be easier to call super and check if the return value is undefined instead of manually checking registration like this? Something like:

let FoundComponent = this._super(...arguments); If (podNames[name] && !FoundComponent) { FoundComponent = Component; owner.register('component:' + name, FoundComponent); } return FoundComponent;

I had tried this it wasn't working for me. I think cause _lookupFactory does more then just return the constructor. I would have to call super again at the end, which calling it twice felt weird.

rwjblue · 2016-03-21T13:28:53Z

This is looking great! I'd love to see this land on master and released (with a major version bump) as a prerelease so folks can start test/using it (and we can track issues/PR's independently).

@rwjblue

…ethod so that they will be done before application instatiation rather then after and only ran once for all tests per @rwjblue suggestion

@rwjblue

… though the application, and using the polyfill for older versions per @rwjblue suggestion

…ps due to the final preprocessing step, and the annoyance of having to create a file even if none existed, I moved to using a persistant output plugin specifically for this purpose. *Note* Would like to move to using a manifest file that is a collection of imports rather then a file that gets concatinated due to better source map support. If there is another way to handle that. Let me know

@stefanpenner

…ons of node per @stefanpenner and @rwjblue suggestions

webark · 2016-03-21T19:02:05Z

@rwjblue and @stefanpenner all of you suggestions have been addressed I believe. @ebryn or @topaxi what do you feel about the suggestion to prerelease this with a major version bump? I think it's read for that. I'm going to be replacing the tests over the next couple of days, and an architecture decision needs to be made about how to support plain css files (if at all) when no preprocessor is included. But it would be nice to get this into master sooner rather then later.

@stefanpenner

…per @stefanpenner and @rwjblue suggestions

ebryn · 2016-03-22T05:02:14Z

I've created a stable branch for the existing 0.1.x code, rebased and merged this PR to master

ebryn · 2016-03-22T05:08:57Z

We've got a beta release for this work as well... [email protected]

stefanpenner reviewed Mar 9, 2016
View reviewed changes

webark added 2 commits March 9, 2016 11:14

using plain init and calling super for better preformance per @stefan…

770538f

…penner suggestion

using a persistant filter so that the output will be cached accross r…

27c9ff0

…eloads for better preformance per @stefanpenner suggestion

using .get for the tagname since it is a member of a ember object, an…

6f16ec8

…d could potentialy be a computed property per per @stefanpenner suggestion

stefanpenner reviewed Mar 18, 2016
View reviewed changes

webark mentioned this pull request Mar 18, 2016

Component subclassing support? #48

Open

webark added 3 commits March 18, 2016 14:18

brings in line with the expected output of ember init

a48f9a2

moving actual dependecies to from dev to dependencies to ensure that …

ce2b691

…they will get included iwth the package

switched generating the pod name json file to use fs-tree-diff and pe…

ce35f16

…rsistentOutput per @stefanpenner suggestion

switched to using glob syntax as per @stefanpenner suggestion

e88ea12

topaxi mentioned this pull request Mar 19, 2016

Compatibility with postcss/cssnext #50

Closed

This was referenced Mar 20, 2016

Build of JS files not working. #103

Closed

Make it compatible with ember 2.5 #124

Merged

rwjblue reviewed Mar 21, 2016
View reviewed changes

webark added 5 commits March 21, 2016 08:46

moved the componentlookup and compont repoens out of the initialize m…

de54b2d

…ethod so that they will be done before application instatiation rather then after and only ran once for all tests per @rwjblue suggestion

using the updated api of calling the methods on the owner rather then…

0f79553

… though the application, and using the polyfill for older versions per @rwjblue suggestion

switched to apply and passing in all arguments per @rjwblue suggestion

82508ce

moved to standard object function declaration to support lesser versi…

82591bc

…ons of node per @stefanpenner and @rwjblue suggestions

moving more function declarations to support lesser versions of node …

586b8e9

…per @stefanpenner and @rwjblue suggestions

ebryn closed this Mar 22, 2016

Updated to maintain roughly the same api, but remove monkeypatches #123

Updated to maintain roughly the same api, but remove monkeypatches #123

Conversation

webark commented Mar 9, 2016

webark commented Mar 9, 2016

webark commented Mar 9, 2016

stefanpenner Mar 9, 2016

Choose a reason for hiding this comment

topaxi commented Mar 9, 2016

webark commented Mar 9, 2016

webark commented Mar 9, 2016

webark commented Mar 9, 2016

topaxi commented Mar 9, 2016

webark commented Mar 9, 2016

topaxi commented Mar 10, 2016

webark commented Mar 10, 2016

topaxi commented Mar 10, 2016

topaxi commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

topaxi commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

topaxi commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

webark commented Mar 10, 2016

stefanpenner Mar 18, 2016

Choose a reason for hiding this comment

webark Mar 18, 2016

Choose a reason for hiding this comment

webark commented Mar 18, 2016

webark commented Mar 18, 2016

rwjblue Mar 21, 2016

Choose a reason for hiding this comment

rwjblue Mar 21, 2016

Choose a reason for hiding this comment

webark Mar 21, 2016

Choose a reason for hiding this comment

rwjblue commented Mar 21, 2016

webark commented Mar 21, 2016

ebryn commented Mar 22, 2016

ebryn commented Mar 22, 2016