atomify / atomify-css

Atomic CSS - Reusable front-end styling using Rework, plugins, and Node's resolve algorithm
MIT License
78 stars 18 forks source link

bower support #30

Closed cellvia closed 10 years ago

cellvia commented 10 years ago

does not break any tests (that werent already broken)

added new set of tests into separate test file -- may be overkill if you want to remove some, or consolidate no problem w/ that from my end

references https://github.com/atomify/atomify-css/issues/29

cellvia commented 10 years ago

sorry, this added a duplicate of rework-clone in the test folder, which is unnecessary. removing and committing.

cellvia commented 10 years ago

let me know if you'd like me to redo the PR to avoid that extra commit or change anything else to get this in

joeybaker commented 10 years ago

Nifty. A few questions:

  1. Is it necessary to checkin all of bower_components? Can we git ignore them, and run a bower install as a prereq of these tests?
  2. Is it necessary to duplicate all the tests? Is there a slimmer test suite that still covers our bases?
cellvia commented 10 years ago

thanks for taking a look :-)

re: 1) they are basically duplicates of the node modules you guys had checked in the tests, just modified to be bower components... they dont actually exist in the bower registry or anything. is there a way in general to collapse a file structure into a scaffolding build script? such would be equally cool to apply to the checked in node_modules ? seems like a lot of work though :-/

re: 2) probably so, import type should only affect a few key tests realistically. want me to take a stab at slimming it? if its small enough i'll just put them straight into test.js. though that scaffold of bower_components files is necessary for one of the most troublesome issues w/ this PR: integrating rework plugins (which are node_modules that live INSIDE bower components) hence the need for the intricate scaffold

joeybaker commented 10 years ago

Thanks for the PR :)

  1. hmm… could a symlink do the trick?
  2. If you wouldn't mind, yes, a slimmed down test suite would be ideal. I'd love not to have to duplicate tests all the time!
cellvia commented 10 years ago

whew well... i could use this: https://github.com/BenJanecke/sane-scaffold

and create a symlink to the build script, shall i put that in /bin ? also what would you like the command to be... buildbower ?

i dont mind slimming down the test suite...the harder part is the above ;-) so if i do these things we are greenlight on the PR ?

joeybaker commented 10 years ago

Yup! We'd like to merge this, just need those two things fixed.

cellvia commented 10 years ago

Ok, last thing, perhaps i should make the bower test file generated separately (test-bower.js perhaps) otherwise the bower tests within test.js will always fail until bowerbuild is run...

joeybaker commented 10 years ago

hmmm… can you have the bower test file run bowerbuild?

cellvia commented 10 years ago

i suppose so, and create a script in the package.json thats like npm run bower-test ?

cellvia commented 10 years ago

updating the bower tests later would not be fun this route however

cellvia commented 10 years ago

theyd be stored as a string in this /bin file (thats how the scaffolding works)

joeybaker commented 10 years ago

You could modify npm test in the package.json to run bowerbuild?

cellvia commented 10 years ago

oh yeah, thats true... would you be ok with that?

joeybaker commented 10 years ago

does bowerbuild cache its result?

cellvia commented 10 years ago

im not sure what you mean... it uses that scaffold script to modify the filesystem in place, adding bower components folders into the fixtures folders...

cellvia commented 10 years ago

hope that works here cause i just did a painstaking code streak just for this! i tested the output and all the tests pass on it. just need to figure out the right way to build it out. im sensing you want it to be totally optional, so perhaps i can have the separate test-bower.js file, but trim it down, and running it (node test-bower.js) will scaffold the files, and then run the test. this will avoid the bin folder / symlink as well. all you'd need is the test-bower.js file.

joeybaker commented 10 years ago

You're right. We want it totally optional, but we do want to run the necessary tests on every test run. I'm fine with having a separate test/bower.js file, but I'd like to avoid having it straight-up duplicate every test. Ideally, it would just run test to ensure that the bower layer works. What do you think?

cellvia commented 10 years ago

to get the closest to that ideal, i am seeing two options:

1) symlink bowerbuild, then have the npm test script be: bowerbuild && tape test.js | tspec then it will build the files, and test.js can then include the much slimmed down bower tests (probably 3 or 4 max)

2),another idea you suggested, have bowerbuild invoked within test.js itself before running the tests. this avoids the need to symlink and avoids altering npm test

either one will build the bower components into the fixtures folder upon running the tests... both will make it optional in that you have to run the test for the build to happen.

does this sound about right?

any preference which one? thanks for the time and thoughtfulness!

joeybaker commented 10 years ago

My preference is for whichever one is faster on multiple runs :)

cellvia commented 10 years ago

this should be good to go now...

techwraith commented 10 years ago

@joeybaker can you take another look at this one?

@cellvia Can you get this mergable again, I think Joey swapped some code out from under you ;)

joeybaker commented 10 years ago

@cellvia just ping me when this is mergable again. I'd be happy to take another pass.

cellvia commented 10 years ago

@joeybaker done :-)

cellvia commented 10 years ago

Any luck???

cellvia commented 10 years ago

requests honored! (with minor exceptions, see notes above) PS i tested this in windows, and bowerbuild is properly creating the folder structures! also to note... while atomify-css fails most of the tests on the windows side, it seems at least like the bower tests are failing in line with the normal tests ie simply due to line break differences between windows and unix

joeybaker commented 10 years ago

Thanks all the work @cellvia!

Windows is a tough one. I'll open an issue so we can track that.

Merging!