davidguttman / sassify

Require scss files in Browserify
MIT License
51 stars 22 forks source link

Import from npm install files #37

Closed konsumer closed 8 years ago

konsumer commented 8 years ago

I would like to be able to import from other projects in node_modules at runtime, using my npm path. The problem is when I have a module installed alongside other modules that are it's dependencies (as with modern version of npm) and so I can't count on the structure of node_modules.

It would turn things like this:

@import "../../../node_modules/material-colors/dist/colors.scss";

which breaks if the module is installed alongside material-colors. The path that would work in that case, but not standalone, would look like this:

@import "../../../../material-colors/dist/colors.scss";

If there was even a way to count on the node_modules dir in a variable, and I could do something like this, it would work ok, even if not ideal:

@import $module_path + "material-colors/dist/colors.scss";

I'd prefer to be able to do something like this:

@import "material-colors/dist/colors.scss";

less/lessify works this way.

This module gets close, but has requirements to setup package.json with the right structure. I made an issue that tracks that, so maybe it will be as simple as adding that module to the node-sass options.

konsumer commented 8 years ago

Actually sass-module-importer appears to already do this, so I guess the issue is including that project in the options for node-sass, like this:

var sass = require('node-sass');
var moduleImporter = require('sass-module-importer');

sass.render({
  file: './source/css/app.scss',
  importer: moduleImporter()
}, cb);

Do you want me to make a PR?

davidguttman commented 8 years ago

That would be great, thanks!

konsumer commented 8 years ago

Should I also increment the npm semver MINOR so you can easily npm publish?

davidguttman commented 8 years ago

Usually, I'd handle the versioning, but either way.

konsumer commented 8 years ago

Ah ok, sorry. I already did it on the PR, but you can just not merge that commit if you don't like the version. Thanks!

konsumer commented 8 years ago

I was getting an error about importer being a function, so I moved it down to the if (options.importer) statement. I will do some more testing and make sure it works ok. This brings to light the issue that options.importer might not be a string, so maybe we should just pass it along without resolving path.

Old code + fix:

   if (options.importer) {
        if ((path.resolve(options.importer) === path.normalize(options.importer).replace(/(.+)([\/|\\])$/, '$1'))) {
            options.importer = require(options.importer);
        } else {
            options.importer = require(path.resolve(options.importer));
        }
    }else{
        options.importer = moduleImporter();
    }

Idea:

    if (!options.importer) {
        options.importer = moduleImporter();
    }
konsumer commented 8 years ago

Seems to work fine with the first fix. Here is a gist to try it out.