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

[Major Changes][Sprockets 3] Supporting multiple sprockets versions and using versionized classes. #39

Merged
merged 2 commits into from
Aug 23, 2016

Conversation

bogdanRada
Copy link
Contributor

@bogdanRada bogdanRada commented Aug 22, 2016

Hello , as discussed here: #38 i made the changes for using different classes for different versions.

Tilt dependency is still removed, but if you want to keep it for the legacy versions , let me know and i can rewrite the V2 class for SASS template.

All tests are passing. for botth sprockets 2.x and 3.x, however sprockets 4 is not support yet, becasue i need to fix the importer for that version

Also fixed all issues i could find with Rubocop.

THIS WILL ALSO DROP Ruby 1.9

Let me know if this is better now :)

@bogdanRada
Copy link
Contributor Author

Of course, some more cleaning can be done,if needed. Let me know if you want to refactor something or change anything. Thanks

@bogdanRada
Copy link
Contributor Author

bogdanRada commented Aug 22, 2016

I think we should also update the Sprockets::Sass::V3::Functions to be the same as the ones from Sprockets-herlpers, some of them were changed to require two parameters for sprockets 3, But i am not sure if we need to keep compatibility with that gem too. Currently the tests are passing because our functions are included after the Sprockets::Helpers so we are overriding them.

This functions will need to get refactored though for sprockets 4, becauase context.logical_path is now throwing NoMethodError.

Just wanted to know if we want to keep that compatibility or not.

Or we can drop our functions for sprockets >= 3 and rely only on sprockets-helpers. Let me know what you decide. Thanks

Actually i think this is intended, seems same thing happens with sprockets 2.x. Nevermind about this :)

@bogdanRada bogdanRada mentioned this pull request Aug 22, 2016
end

def require_libraries
AVAILABLE_VERSIONS.each do |version|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this only require the the libraries for the current version of sprockets? If we're requiring all of them, we could just explicitly list the require statements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually yes, but considering for example for sprockets 4....from the list of files only three of them are currently defined for that version....the other ones will be used from v3...since no changes are there.

@bogdanRada
Copy link
Contributor Author

I updated the code as you suggested, all tests are passsing, I am still working on V4 Importer.

@bogdanRada
Copy link
Contributor Author

bogdanRada commented Aug 23, 2016

I improved a bit the v4 importer....now only 9 failures are there...but i am still working on the other failures. The other tests for sprockets 2.x and 3.x are still passing. https://travis-ci.org/bogdanRada/sprockets-sass/builds/154435384

Edited: This is the latest build https://travis-ci.org/bogdanRada/sprockets-sass/builds/154467954 and now are only 6 failures for sprockets 4.x

@bogdanRada
Copy link
Contributor Author

I hope now is better, I can rebase and squash the commits if you approve the changes. Let me know if i need to change something else . THanks

@petebrowne
Copy link
Owner

petebrowne commented Aug 23, 2016

Thanks, this is really great. What's left todo?

  • sprockets-helpers compatibility
  • sprockets 4 compatibility

^ Both of these could be separate PRs

@bogdanRada
Copy link
Contributor Author

bogdanRada commented Aug 23, 2016

Yes, i agree, and some real life testing on applications, to see if this is working as expected.

I rebased the commits and squashed them into a single commit.

Also i have a question, seems that in newer versions of sass , is possible to use render_with_sourcemap from the Sass::Engine class. Do you need some support for rendering with Sourcemap also?
That can be done in a separate PR also if needed.

Let me know if something else is needed from this PR, refactoring something or changing anything. Thank you very much for your support.

@bogdanRada
Copy link
Contributor Author

I tested with older version of sprockets 2.12 and some other versions (2.x) , they seem to work fine. However i don't have an application with Sprockets 3.7 ready . I can try and test with latest version also but will need some time to prepare an application for that if is necessary.

@petebrowne
Copy link
Owner

👍 Merging this in - I'm planning to release this as 2.0.0.beta1 so we can have people test it out in real world applications while we iron out some of the other details.

@petebrowne petebrowne merged commit f506031 into petebrowne:master Aug 23, 2016
@bogdanRada
Copy link
Contributor Author

Wow....thanks very much :) This is awesome :D

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

Successfully merging this pull request may close these issues.

2 participants