allow importing JS files too

[vieron]

cc/ @pmowrer

[pmowrer]

@vieron Thanks for the PR! I've already turned down a request for the same functionality but perhaps it should be reconsidered. My hesitation is that "importing a JS file" can mean so many different things. In both this PR and the issue referenced, "importing a JS file" essentially means importing JSON format exported via CommonJS in a JS file, but if someone tried to import a "regular" JS file, the importer would break. By contrast, JSON is a much stricter format and easier to reason about.

What's your use-case? Why not just make it a JSON file to begin with?

[vieron]

Hi @pmowrer!

The reasons why I prefer JS over JSON are:

• Be able to write comments!
• Import another node modules, shared configs, built-in modules, etc...
• Do math with numbers
• Avoid a big nested tree (JSON), in favor of separate module.exports

I agree importing JS isn't as robust as JSON, and it could break the importer. But we could JSON.stringify and then JSON.parse the value of module.exports inside a try catch, to give feedback the user, and to ensure we'll always pass a valid JSON object to your parseJSON function. Something like...

var contents;

try {
    contents = require(file);
    if (isJSFile) {
        contents = JSON.parse(JSON.stringify(contents));
    }
} catch(e) {
    // log error
    contents = {}; // instead of return an empty object we could break the build
}

return {
    contents: parseJSON(contents)
};

What do you think?

[pmowrer]

I think those are good points. My main hesitation is that if someone wrote another importer that targeted JS files for some other reasons, they wouldn't be able to use this importer at the same time b/c they'd block each other.

A way of limiting that type of collision would be to return sass.NULL in the catch statement, basically saying that unless the JS file exports CommonJS in a JSON format, ignore it.

[vieron]

Thanks for the answer @pmowrer!
Let me perform those changes. I'll update the PR in the next days.

[vieron]

Sorry, I read too fast. If you don't feel comfortable having js support in this importer, I can move it to a separate repo and name it node-sass-js-importer. Let me know :)

[pmowrer]

I think it makes sense in this importer so long as we ignore JS files that aren't exporting JSON.

[vieron]

Ready for review!

[pmowrer]

This test title doesn't seem to match the test expectation ("strip" vs "throw")

[vieron]

You are right!

[pmowrer]

Closing due to inactivity