[Major Refactoring] [Sprockets 3]

[bogdanRada]

Hello,

As per discussion here #36

This pull request tries to add support for sprockets 3, i made a lot of refactoring , and updated the Readme for using the compressor .

Also checked the code with Rubocop and there are no issues. Just one about not rescueing LoadError, but i think that is ok to leave it as it is.

This also drop Tilt as a dependency, which should fix this issue #35

Also this also fixes this issue: #24 , by adding the license to the gemspec and also improved a bit the gemspec :)

Please review the changes, and let me know if it is ok . The code might be rough to understand, i tried to structure it as best as i could

I still need to add more tests , and documentation, but unfortunately i don't have so much time,
I will be available for discussion next week, after that i am going to be travel for 3 weeks.

I wanted to open this pull request to get some feedback at least . All tests are passing from all appraisals, You can see the latest Travis build here: https://travis-ci.org/bogdanRada/sprockets-sass/builds/153545395

There are still some issues with Sprockets 4, it doesn't fully suppport it , i am going to still try and work on that and will probably open a new pull request when i have some updates regarding that.

Also I am not sure if this is the place to ask,,,,but if you want you can add me as collaborator after merging this pull request in case there are issues. I would gladly help. Just wanted to say that. Is really your decision, i only wanted to offer my support though.

Please be gentile when reviewing the code, i know some of the code might not be as 100% bullet-proof. Due to this changes, i know that the performance might be affected, i haven't checked though, i am not even sure exactly how to measure that.

I tested this with a personal Sinatra application and it worked ok. Tested with older sprockets 2.12 and worked fine.

Let me know if some changes, improvements or fixes need to be done. Have a nice day :)

[petebrowne]

The second condition here seems unnecessary

[bogdanRada]

yes, that's true

[petebrowne]

So taking a step back here...would it be better if dropped support for Sprocket 2.x and released this a backwards incompatible upgrade? The code to support both is very complex

[bogdanRada]

I saw your comments but i am not going to be able to fix those issues until monday or answer your questions...i am travelling right now...and i am on my phone.

I know is a bit complex but a lot of people are still using sprockets 2.x including myself. I am not sure if we should drop support for sprockets 2.x becauase this gem is a depedency to other gems and the sprockets team could potentially release new versions for those if there is a bug, like the rails team does sometimes.

Honestly with sprockets 4.x the code would get even more complex in the importer ... A lot of changes will be needed there and a lot of refactoring.

I think the best thing i could do is to split the responsabilities between classes to have different classes for different versions. This way we could maintain all versions. Theoretically at least.

But is your decision. Let me know what you decide so that i can make the necesary changes.

[petebrowne]

Here are the options as I see it:

1. Support Sprockets 2-4 (and beyond?) in the same codebase. I think if we go this route, your idea of splitting the responsibilities between classes is probably the easiest to maintain.
2. Create separate versions of sprockets-sass that support a specific version of sprockets. And come up with support plan for each version. Something like:
   • sprockets-sass 2.x (bump to 2 for consistency) - will support sprockets 2-3 - will get bugfixes, but no new features
   • sprockets-sass 3.x - will support sprockets 3-4 - will get bugfixes + features
   • sprockets-sass 4.x - will support sprockets 4-5, etc.

[bogdanRada]

I would definitely go with the first option. The second one means building at least 5 gems ( one would be the core ) but honestly people tend to not like adding more dependencies to their apps and it would be even harder to maintain all of these gems.

I think having same codebase is better , You have more control and its easier to maintain and also we would avoid conflicts between so many gems.

I would need though few days to make those changes and re-structure everything.

If You want You can close this and will open another pull when i am Done.

But please do let me know if this is your decision. I am ok though with the third option also of dropping support for sprockets 2.x .

I am not sure though which is the best. My Main concern is that other gems are dependent on this. If we want to drop support for sprockets 2....then i am not sure how they are going to Be affected.

Either way i am opened to a fourth option also if You have other ideas.

[bogdanRada]

Sincer we are discussing dropping stuff.. would You also consider dropping support for Ruby 1.9.x since is not maintained anymore ? And use ruby greater or equal to 2.0? Would help me If i could know this. Just wanted to ask this also.

[petebrowne]

The second one means building at least 5 gems ( one would be the core )

Oh I think I didn't explain myself well - I meant we would create different versions of the sprocket-sass gem for each corresponding sprockets version we would like to support. We'd only release/maintain one gem and project. It would be even easier for other gems that use sprockets-sass as a dependency to determine which one to support. I.e., if they're using sprockets 3, then they'd use sprockets-sass 3.

You also consider dropping support for Ruby 1.9.x since is not maintained anymore?

Yeah, that's fine - but we'll have to release it as major version bump, since that'll be a backwards incompatible change

[bogdanRada]

I am not sure about that. I honestly don't really like the idea ...because honestly during the research for this i often fiind that sprockets team would rename move refactor or simply delete methods that we were actually using. My Main concern is that if we start releasing version 2.x.x for sprockets 2. Version 3.x.x for sprockets 3...and version 4.x.x for sprockets 4....we could definitely keep this for the legacy versions where no major changes will be Done . But for the newer versions if they will change the code base as often as they did until now....i am afraid that we would not be able to maintain same versioning....because even a small change în one of their internal methods....could mean a major change în our gem ....which would need to bump our gem to a even other major version.

This is because în the importer we use a lot of internal methods from sprockets that are subiect to change at any given time.

În order to avoid such situations i think would be just enough to split the responsabilities between classes by different version and just bump the version normally .

But is really your decision. I am going to start working on splitting the logic to different classes.
I haven't got yet time to work on that...i am still out of town but will work tomorrow.

I Hope i will finish before the end of the week. After that i will be out for three weeks.

I honestly think that is better to support all versions în a single gem because i think Most gem creators usually prefer to Be backward compatibile with other versions than supporting just one version. And this would make everyone happy since we would support multiple sprockets versions and they could activate any of them based on the sprockets version loaded.

I would make sense to do that versioning for applications like rails or Sinatra were only one version of sprockets can be active at a given time.

But we should consider also the other gem creators that might have some plugins being dependent on this gem .

Maybe i am not seeing the whole picture or maybe i am desconsidering something. But this is my opinion

I am Glad though that You agree dropping suport for Ruby 1.9.x . Will let You know when i make more progres on that refactoring.

[bogdanRada]

hmm, i started working on the refactoring...but i am not sure what to do about sprockets 2.x....do we still want to keep Tilt as dependency ?

[bogdanRada]

Did the changes as requested here in another pull request #39 . I am going to close this one and will discuss further there . Thank you very much.