Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Misc. Fixes #660

Merged
merged 4 commits into from
Feb 11, 2015
Merged

Misc. Fixes #660

merged 4 commits into from
Feb 11, 2015

Conversation

am11
Copy link
Contributor

@am11 am11 commented Feb 10, 2015

  • Build: Use node-gyp from dependencies.
    • Code: Minor refactoring.
  • CI: Tests io.js runtime.
    • Promotes node.js to v1.2.
  • Test: Adds outFile related tests.
  • Test: Adds sourceMap path related tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.18%) to 92.77% when pulling cebf678 on am11:master into fe1d59f on sass:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.79%) to 96.39% when pulling 60031fb on am11:master into fe1d59f on sass:master.

@am11 am11 force-pushed the master branch 8 times, most recently from 98162cd to 5a31a68 Compare February 10, 2015 21:03
@coveralls
Copy link

Coverage Status

Coverage increased (+5.0%) to 97.59% when pulling 5a31a68 on am11:master into fe1d59f on sass:master.

@am11
Copy link
Contributor Author

am11 commented Feb 10, 2015

Coverage!!! ⚡

@am11 am11 changed the title Build: Use node-gyp from dependencies Misc. Fixes Feb 10, 2015
@austinpray
Copy link

@am11 keep going!! 100%%% coverage or go home

@xzyfer
Copy link
Contributor

xzyfer commented Feb 10, 2015

Can you give some context to what you mean by

Use node-gyp from dependencies

@am11
Copy link
Contributor Author

am11 commented Feb 11, 2015

@xzyfer, for example I used to run node-gyp rebuild or node scripts/build -f. Both of these use node-gyp installed at global scope (npm install -g node-gyp). Also, node-gyp was not in our package dependency. There are two problems with this:

  • only pangyp (a fork of node-gyp) supports io.js at the moment (soon official node-gyp will bring the support as well; there is an open PR).
  • people seldom update their global installations.

With this change, all you need to do is to run node scripts/build, node scripts/build --force (or -f) or node scripts/build --debug (or -d). You can use both -f and -d flags (-fd or -df isn't supported though). This will execute (shell out) runtime-aware build command.

@coveralls
Copy link

Coverage Status

Coverage increased (+6.19%) to 98.78% when pulling b48d5d6 on am11:master into fe1d59f on sass:master.

@am11
Copy link
Contributor Author

am11 commented Feb 11, 2015

io.js is failing because it is building binary with io.js runtime but npm test job is agnostic of runtime at the moment. That would be a complicated solution to make CIs scripts runtime-aware. Something like: koajs/koa@6d5f506.

Also something has changed in io.js v1.1 (and perhaps corresponding libuv), due to which it is failing some tests locally as well. (was passing all with io.js v1.0.x though).

Nonetheless, we can build io.js 1.1 binaries for node-sass 2.0.0 with pangyp (node scripts/build). 😎

@am11
Copy link
Contributor Author

am11 commented Feb 11, 2015

@austinpray that is the plan! :)

Just gained a bit more with this diff:

- try { 
-  err = JSON.parse(err); 
- } catch (e) { 
-  err = { message: err }; 
- } 
- 
+ err = JSON.parse(err); 

Note that I removed try-catch because we don't need defensive programming. It is an "interface promise" by LibSass API that the errors shall be returned in JSON format. If there is an anomaly then the bug belongs to LibSass. 🪲

@xzyfer
Copy link
Contributor

xzyfer commented Feb 11, 2015

Thanks for the clarification @am11. The global node-gyp caught me out recently as well. This is much better.

@am11 am11 force-pushed the master branch 5 times, most recently from 47dcb7d to c1d2ffe Compare February 11, 2015 01:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants