Use sass binary url from .npmrc if available

[fh1ch]

This PR adds the possibility to configure the sass-binary URL in the .npmrc configuration file (e.g. sass_cdnurl=http://example.com/sass-binaries). This is also used the same way in PhantomJS by pointing the download URL to a mirror without having to set an environment variable manually.

[huerlisi]

Cool, that looks like something we've been looking for!

[saper]

That's cool, but let's get at least naming consistent. #1143 is a nice attempt to document those.

Also in the implementation we should just wrap the names of all parameters with npm_config_ and look them up in the environment.

[fh1ch]

Totally agree. I hope npm_config_sass_binary_site is ok as name. I'll also take care of the documentation and add it to this PR.

The second point I didn't really get. What's your idea there?

[xzyfer]

Firstly, thanks for your contribution @fh1ch, we appreciated.

Secondly, some house keep. I the future it's preferred you open an issue to discuss new features before implementing them. Odds are something similar has previously been discussed, or the team may already have plans in the area of interest.

With regards to configuration we do have plans for standardising on nodeSassConfig in package.json to address concerns we have about code portability. In this case however it doesn't preclude your proposed addition.

---

Specifically to this PR we currently support 3 ENV variable that affect the binary download and/or resolution process, BINARY_PATH, BINARY_SITE, and BINARY_NAME. This PR should add support for all these variables, as well test coverage for these new options.

[saper]

I :cupid pull requests :)

@fh1ch check this out https://github.com/sass/node-sass/blob/master/scripts/build.js#L133-L136

• we just transform the same string into the command line option and the environment variable name.

One just needs probably to stick || process.env['npm_config_' + subject] somewhere to make it work as you propose.

[xzyfer]

we should just wrap the names of all parameters with npm_config_ and look them up in the environment.

@saper I also do not understand what we're accomplishing here. I assume you're referring to the following:

Any environment variables that start with npm_config_ will be interpreted as a configuration parameter. For example, putting npm_config_foo=bar in your environment will set the foo configuration parameter to bar. Any environment configurations that are not given a value will be given the value of true. Config values are case-insensitive, so NPM_CONFIG_FOO=bar will work the same.
Source - https://docs.npmjs.com/misc/config#environment-variables

This however isn't adding .npmrc support which I think is worth while.

We already support .npmrc for proxy settings so extending to the binary resolution make sense.

[xzyfer]

One just needs probably to stick || process.env['npm_config_' + subject] somewhere to make it work as you propose

I'm not sure I follow this reasoning. We already have support for the --sass-binary-site and sass-binary-path flags. The npmconf module should handle the npm config resolution process i.e. npm_conf_foo vs foo on .npmrc

[fh1ch]

I applied the .npmrc configuration possibility also to the other environment variables as proposed by @xzyfer. The're now also covered by documentation and runtime tests.

[xzyfer]

This test should be added in addition to the existing package.json tests. The test you're removing here is still important.

[fh1ch]

I tried to avoid redundancy there. The test is still existing but tests that .npmrc parameters can override package.json parameters ( see checking if .npmrc configuration variable overrides package.json). And checking if environment variable overrides .npmrc configuration variable checks if env parameters can override .npmrc parameters. This covers the whole chain form a hierarchical point of view.

I can of course add the original tests again if you like.

[xzyfer]

I don't think either approach is correct. We now have 4 ways to set these configurations. Testing they get set is important, testing their precedence is most important. The better tests would be asserting, per variable, that precedence is respected.

describe('configuration precedence should be respected', () => {
  describe('SASS_BINARY_PATH', () => {
    beforeEach(() => {
      process.env.SASS_BINARY_PATH = 'foo';
      process.env.npm_config_sass_binary_path = 'bar';
      require.cache[packagePath].exports.nodeSassConfig = { binaryPath: 'baz' };
      require(extensionsPath);
    });
    it('environment variable', () => {
      assert.equal(process.sass.binaryPath, 'baz');
    });
    it('npm config', () => {
      process.env.npm_config_sass_binary_path = null;
      assert.equal(process.sass.binaryPath, 'bar');
    });
    it('package.json', () => {
      process.env.npm_config_sass_binary_path = null;
      require.cache[packagePath].exports.nodeSassConfig = {};
      assert.equal(process.sass.binaryPath, 'foo');
    });
  })
})

[fh1ch]

You're right, this is a much better solution. I will start refactoring all configuration unit tests.

[xzyfer]

You're awesome!

This should also reduce some bloat from these specs! \o/

[fh1ch]

The unit tests are now implemented in the way you suggested. It's now much smaller and cleaner without decreasing the coverage. I'm hoping your happy with the current solution. Just tell me if there is anything else to do.

[xzyfer]

Amazing work! 😍

[fh1ch]

Thank you very much, I'm really humbled 😍 I'm glad if I can help to bring this awesome project a bit forward.

[xzyfer]

If you're comfortable with git rebase I'd ask you to please squash these commits together

• Use sass binary url from .npmrc if available
• Adapted all variables to support aligned .npmrc parameters

and

• Added runtime tests for .npmrc parameters
• Refactored runtime unit test to follow new configuration schema

---

I the end there would be 3 commits

• A commit adding .npmrc support
• A commit updating the tests
• A commit updating the docs

[saper]

I think it's --sass-binary-name. At least I hope so :)

[fh1ch]

Yes, you're right. Thank you very much for the input. I will correct and push it when I'm rebasing the stuff.

[saper]

On Wed, 16 Sep 2015, Michael Mifsud wrote:

• Use sass binary url from .npmrc if available
• Adapted all variables to support aligned .npmrc parameters

and

• Added runtime tests for .npmrc parameters
• Refactored runtime unit test to follow new configuration schema

---

I the end there would be 3 commits

• A commit adding .npmrc support
• A commit updating the tests

I don't mind having those two together, so that we are 100% green
between commits.

[fh1ch]

No problem, I will quickly rebase that stuff as you two suggest it.

[xzyfer]

Presumably the existing test should still pass with the new functionality.

To be honest I'm fine squashing this all into 1 commit. It's all tightly related.

[fh1ch]

There are now only 3 commits. Should a rebase them all into 1?
Would make sense since Travis striked again 😄

[saper]

On Wed, 16 Sep 2015, Fabio Huser wrote: There are now only 3 commits. Should a rebase them all into 1? Would make sense since Travis striked again 😄

I am just trying to quickly fix some bug in Travis tests so I'd love more testing :)

[fh1ch]

On Wed, 16 Sep 2015, Fabio Huser wrote:
There are now only 3 commits. Should a rebase them all into 1?
Would make sense since Travis striked again 😄
I am just trying to quickly fix some bug in Travis tests
so I'd love more testing :)

Good to now 😄 Are these 3 commits now ok for you or should I squash them all together?

[saper]

On Wed, 16 Sep 2015, Fabio Huser wrote:

On Wed, 16 Sep 2015, Fabio Huser wrote:
There are now only 3 commits. Should a rebase them all into 1?
Would make sense since Travis striked again 😄
I am just trying to quickly fix some bug in Travis tests
so I'd love more testing :)

Good to now 😄 Are these 3 commits now ok for you or should I squash them all together?

As they say those days, LGTM!

[fh1ch]

Awesome! Just tell me if I can help with something.

[xzyfer]

@fh1ch this is all good! Our plan is to ship in 3.4.0 hopefully in a couple weeks.

The reason for 3.4.0 is that this is feature and by semver it should be a minor release.
The reason for not releasing 3.4.0 immediately is that we want 3.4.0 to have LibSass 3.3.0-stable.

This is largely to help communicate to people which version of node-sass has which version of LibSass.
Also because LibSass follows pre 1.0.0 semver atm so 3.3 is actually a major version bump from 3.2.

We really appreciate your work and we're super keen to get it out in the wild ASAP.
We expect to have 3.4.0-beta out this week!

[fh1ch]

@xzyfer: Good to know, thanks 😄 It doesn't hurries for me so it's not that important in which release it gets. But it's nice to have libSass-3.3.0 in.

Thank you very much guys, it was a pleasure for me contributing to this project.