Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #775: Introduce SASS_BINARY_SITE environment variable #835

Closed
wants to merge 1 commit into from

Conversation

saper
Copy link
Member

@saper saper commented Apr 4, 2015

Provide ability to locally mirror node-sass
binaries for various versions and platforms.

SASS_DIST_SITE needs to be an URL pointing to
a collection of files organized like the Github
repository.

If SASS_DIST_SITE is set to

http://myhost:8080/local/node-sass-bin

then

http://myhost:8080/local/node-sass-bin/v3.0.0-beta.4/freebsd-x64-14_binding.node

should point to the FreeBSD 64 bit binary for node 0.12.0

@am11
Copy link
Contributor

am11 commented Apr 4, 2015

We already have binaryUrl in nodeSassConfig for this purpose.

@@ -89,8 +89,9 @@ function getBinaryUrl() {
return flags['--sass-binary-url'] ||
package.nodeSassConfig ? package.nodeSassConfig.binaryUrl : null ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can augment this with ['v', package.version, '/', sass.binaryName].join('')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can; but this changes the meaning of SASS_BINARY_URL variable.

If somebody set it like this

SASS_BINARY_URL=http://somehost:8080/local/freebsd-x64-14_binding.node

it will no longer work, instead they would need to re-organize their files under 'v' + package.version to make this work again.

We can deprecate the SASS_BINARY_URL variable if we want, but let's give a new one a new name and let's warn users that the old one will no longer work.

This can be very confusing when somebody is upgrading a larger set of machines with npm...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Then lets replace sassBinaryUrl with sassBinarySite (or preferably sassBinaryCdn as per the original requester #720) with the following precedence order:

  • CLI argument --sass-binary-cdn
  • nodeSassConfig.sassBinaryCDN
  • process.env.sassBinaryCDN

If none of them are found, it will fall back to default github URL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should once migrate to something like https://github.com/dominictarr/rc or https://github.com/npm/npmconf so I'd love to have some consistent config variable names and camelCase makes it a bit difficult...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saper please read my previous comment again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I read. Btw. --sass-binary-url didn't work at all, it caused an exception (in current master).

@saper saper changed the title Fix #775: Introduce SASS_DIST_SITE environment variable Fix #775: Introduce SASS_BINARY_SITE environment variable Apr 9, 2015
@saper saper force-pushed the fix/775 branch 3 times, most recently from 297f93c to a371de9 Compare April 9, 2015 22:33
*
* @api private
*/

function getBinaryUrl() {
return flags['--sass-binary-url'] ||
package.nodeSassConfig ? package.nodeSassConfig.binaryUrl : null ||
(package.nodeSassConfig ? package.nodeSassConfig.binaryUrl : null) ||
process.env.SASS_BINARY_URL ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have sassBinarySite, we do not need sassBianryUrl and the test in api.js that goes with it.

@am11
Copy link
Contributor

am11 commented Apr 11, 2015

You can port the related test from other PR.
Otherwise LGTM 👍

@xzyfer
Copy link
Contributor

xzyfer commented Apr 22, 2015

What do you think about test refactoring? #775 (comment)

I'm not sure I have context here. Are you referring to #835 (comment)?

If so I don't what PR he is referring to. Please fill me in.

@saper
Copy link
Member Author

saper commented Apr 22, 2015

Sorry. It is #847. I found it very boring to copy paste tests to test this one (#835)

(
flags['--sass-binary-site'] ||
(package.nodeSassConfig ? package.nodeSassConfig.binarySite : null) ||
process.env.SASS_BINARY_SITE || 'https://github.com/sass/node-sass/releases/download'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the default url into package.json

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correct could we not simply default package.nodeSassConfig.binarySite to 'https://github.com/sass/node-sass/releases/download'?

Provide ability to locally mirror node-sass
binaries for various versions and platforms.

SASS_BINARY_SITE needs to be an URL pointing to
a collection of files organized like the Github
repository.

If SASS_BINARY_SITE is set to

 http://myhost:8080/local/node-sass-bin/

then

 http://myhost:8080/local/node-sass-bin/v3.0.0-beta.5/freebsd-x64-14_binding.node

should point to the FreeBSD 64 bit binary for node 0.12.0

The URL can be also specified as the --sass-binary-site
commandline option or in the package.json:

 "nodeSassConfig": {
   "binarySite": <url>
 }

Remove --sass-binary-url and friends.

Also change priority and use package.json defaults
last, after command line arguments and the environment.
@xzyfer
Copy link
Contributor

xzyfer commented Apr 27, 2015

#847 appear to have this patch included. Closing this.

@xzyfer xzyfer closed this Apr 27, 2015
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Remove the non-standard compact function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants