petebrowne / sprockets-sass

Better Sass integration with Sprockets 2.x
MIT License
90 stars 29 forks source link

Don't include the context in the importer #26

Closed bhollis closed 10 years ago

bhollis commented 10 years ago

Sass 3.3 attempts to marshal the importer, resulting in messages like:

Warning. Error encountered while saving cache ./.sass-cache/6d19918aae9b194893adf3de06101cc3ab87507f/_partial2.css.sassc: can't dump anonymous class #<Class:0x007fb885d0a030>

I opened an issue with Sass (nex3/sass#1196) and they said to just make sure the importer is marshal-able, which in this case means not holding a reference to the context. See sstephenson/sprockets#542 for what they had to do.

petebrowne commented 10 years ago

I'm digging through this now and having a bit of trouble. Their solution was to drop the Importer altogether, which removes quite a bit of functionality.

The timeframe for this might be slow, I could use some help if you have the time

petebrowne commented 10 years ago

From what I can tell:

It's necessary that we associate and importer with each source range in order to properly generate source maps. Importers have specific logic about the URIs that should be included in a source map, and we need to access that logic when generating source maps.

Source maps aren't even a supported feature of Sprockets::Sass and would take quite a lot of black magic to support: sass-rails-source-maps.

Is the warning output the only issue you've seen related to this?

petebrowne commented 10 years ago

I've got a branch: sass-3.3 where I'm testing out removing the Importer. There are a few features sprockets-sass will lose as a result.

What do you think? Are these features being used by middleman?

bhollis commented 10 years ago

Thanks for taking a look.

I'm not sure about those features - for example, importing other files seems pretty important, or am I missing something about that? Globs I'm not as interested in, but I'm sure our users use them all over the place.

I'm also interested in actually supporting source maps at some point, so anything that closed that off is a bummer too.

I wonder if there's a way to actually marshal the importer by implementing custom dump/load methods?

petebrowne commented 10 years ago

I'm going to try @nex3's suggestion this weekend. Didn't realize you get the context from the options. Hopefully I'll have a fix here soon

bhollis commented 10 years ago

Awesome, thanks for working on this.

petebrowne commented 10 years ago

Just pushed 1.1.0 which fixes this problem! Let me know how this works for you

There are several improvements I can make to the Importer to make it even more Sass 3.3 compatible.