ImportNode prevents dumping when used with sass-rails #1028

Closed
cheald opened this Issue Nov 30, 2013 · 20 comments

Projects

None yet
@cheald
Contributor
cheald commented Nov 30, 2013

Sass::Tree::ImportNode contains a reference to Sass::Rails::Importer which is undumpable for several reasons as described here: rails/sass-rails#36

This fixes it nicely:

module Sass
  module Tree
    class ImportNode < RootNode
      def _dump(f)
        Marshal.dump([@imported_filename, children])
      end

      def self._load(data)
        filename, children = Marshal.load(data)
        node = ImportNode.new(filename)
        node.children = children
        node
      end
    end
  end
end
@dpehrson dpehrson referenced this issue in Compass/compass-rails Nov 30, 2013
Closed

Rails 4.0, how to make it work #108

@nex3
Contributor
nex3 commented Dec 7, 2013

From the documentation of Sass::Importers::Base:

Importers should be serializable via Marshal.dump. In addition to the standard _dump and _load methods, importers can define _before_dump, _after_dump, _around_dump, and _after_load methods as per Sass::Util#dump and Sass::Util#load.

It's important that an import node preserves the information about which importer imported it. Otherwise relative imports from the imported files will break.

@nex3 nex3 closed this Dec 7, 2013
@spohlenz
Contributor
spohlenz commented Dec 7, 2013

@nex3 Do you have any thoughts or recommendations to add to sstephenson/sprockets#507?

@nex3
Contributor
nex3 commented Dec 7, 2013

Looking into this more thoroughly, I think I may be wrong: we may not be serializing the importer at all, even now. Before we store anything in the cache, we empty out the options, including the importer. Which raises the question of why you're seeing serialization errors with Sass::Rails::Importer if it's not there when the tree is being serialized. Is there a stack trace for one of these errors I can look at?

@nex3 nex3 reopened this Dec 7, 2013
@cheald
Contributor
cheald commented Dec 7, 2013

I'll get a stack trace for you in a bit -- I have to run out for the time being though. I'll circle back on this tonight or tomorrow afternoon.

@cheald
Contributor
cheald commented Jan 7, 2014

So sorry - I managed to completely forget about this. Let me get that stack trace for you.

@nex3
Contributor
nex3 commented Feb 1, 2014

Thanks to @nyarly and @IdahoEv on #1093, I've managed to track down what's going on here. It doesn't have anything to do with options at all. When we parse a Sass file, we attach source range information to each parsed node. These source ranges have enough information to determine precisely where the parsed node came from, which includes the importer that produced that node. It's marshaling these nodes that's causing the problem.

The question now is what to do about it. I don't think it's possible to eliminate the need for importers to be marshalable without making a breaking change to the importer API. The precise importer instance that existed when the node was created has to be available later to provide a public URL for the file in question -- a URL that may be based on the location of the sourcemap, which isn't known at cache-time.

We could add an additional method to Importer::Base to provide a means to associate importers with their source ranges, but in order to avoid serializing any importers this method would need to be implemented by all of them. That means that older importers would break with newer versions of Sass, which isn't something we want.

The final possibility is that Sprockets' importer make itself serializable. I get that it has unserializable context (although I'm not entirely clear on why), so perhaps the way to do this is to -- hackily -- rely on global state. If the context can be made globally available, SassImporter can implement marshal_dump and marshal_load to make itself serializable while it's cached.

I'm closing this issue, since I don't think there's anything Sass core can do about this. If anyone has any ideas, though, by all means let me know.

@nex3 nex3 closed this Feb 1, 2014
@nyarly
nyarly commented Feb 8, 2014

Reviewing the situation on the Sprockets side, it seems like the Sprockets::SassImporter could _load without it's context object (which it uses so that on @import it can register the SASS file as a dependency of the parent asset bundle) and still fulfill the role of an Importer.

But looking at it like that, aren't Importers being used for two completely separate concerns: on the one hand, to locate and load source files, and then (having gone through a marshalling round trip) to satisfy requests generated by the browser from a source map?

In short, the Sprockets Importer could be made to marshal back in a lobotomized form - since, as I understand it, on the other side of the marshalling, the importer no longer needs to update processing dependencies - but aren't Importers doing double duty here? Shouldn't there be a separate "SourceMapper" or something?

@nex3
Contributor
nex3 commented Feb 10, 2014

@nyarly An importer is in charge of knowing everything about how to resolve a set of URIs into a Sass source file. Part of that is saying how that source file can be accessed from a browser. I don't think it makes sense to split these out into two separate classes when the resolution is going to be logically similar much of the time.

@mikeyhogarth

Does anyone know the status of this bug currently? All the open tickets seem to be closed. It happens right out of the box on 4.1.0.rc1;

rails new blog
rails g controller home index
(in routes.rb) root "home#index"

Start server, visit localhost:3000 and you see the message once for each sas file. If you use something like foundation that has loads of files it's really disruptive!

@nex3
Contributor
nex3 commented Mar 10, 2014

@mikeyhogarth sstephenson/sprockets#507 is the bug to track for this.

@mikeyhogarth

Cheers I'll take a look

@noahhaon noahhaon referenced this issue in activescaffold/active_scaffold Jul 28, 2014
Closed

Getting lots of "can't dump anonymous class" sass errors #358

@larslevie

I've seen Issues related to this opened on sass, sprockets, and sass-rails projects. The maintainers of each project are throwing it around like a hot potato. Does anyone know if there is an active Issue open anywhere to try to resolve this?

@sandelius

@larslevie Hallelujah +1

@svoynow
svoynow commented Aug 25, 2014

+1 here too
seeing this now with:
sprockets (2.12.1)
sprockets-rails (2.1.3)
sass (3.4.0)
rails (4.1.1)

@Fudoshiki

rails 4.2 master, sass master, sprockets 2.x
have issue too

@astkaasa
astkaasa commented Sep 1, 2014

sprockets (2.11.0)
sprockets-rails (2.0.1)
sass (3.4.1)
sass-rails (4.0.1)
rails (4.0.4)
seems that the issue comes back again?

@kaishin
kaishin commented Sep 3, 2014
sprockets (2.12.1)
sprockets-rails (2.1.3)
sass (3.3.8)
sass-rails (4.0.1)
rails (4.1.5)

Still having the issue here.

@svoynow
svoynow commented Sep 5, 2014

Should somebody open a new issue? Maybe this one isn't being seen since it's been closed for several months.

@adamjgrant

Bump

@nex3
Contributor
nex3 commented Sep 12, 2014

I'm locking this issue. I've made it clear in comments above that this is a bug in Sprockets, not in Sass.

@nex3 nex3 locked and limited conversation to collaborators Sep 12, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.