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

render error callback not called on import path error #1592

Closed
maxlath opened this issue Jun 13, 2016 · 10 comments
Closed

render error callback not called on import path error #1592

maxlath opened this issue Jun 13, 2016 · 10 comments

Comments

@maxlath
Copy link

maxlath commented Jun 13, 2016

following the removal of working directory-relative import (which I guess is the breaking change mentioned in #1474 ), compiling my old scss code rejected an error of the kind:

{
  "formatted": "Error: File to import not found or unreadable: ../bower_components/foundation/scss/foundation/components/alert-boxes\n       Parent style sheet: /home/maxlath/inventaire/client/app/modules/general/scss/foundation/_imports.scss\n        on line 29 of app/modules/general/scss/foundation/_imports.scss\n>> @import \"../bower_components/foundation/scss/foundation/components/alert-boxes\n   ^\n",
  "message": "File to import not found or unreadable: ../bower_components/foundation/scss/foundation/components/alert-boxes\nParent style sheet: /home/maxlath/inventaire/client/app/modules/general/scss/foundation/_imports.scss",
  "column": 1,
  "line": 29,
  "file": "/home/maxlath/inventaire/client/app/modules/general/scss/foundation/_imports.scss",
  "status": 1
}

But while I do get this error from the executable node-sass/bin/node-sass, I never get it from the module itself: binding.render is called, but neither the error nor the success, letting the callback passed to render hanging. Any clue where this difference of behavior comes from?

@maxlath
Copy link
Author

maxlath commented Jun 13, 2016

I made a demo project of the error to illustrated a related issue in another project but that could be illustrative here too: you can see how sass-brunch is unable to pass an error to brunch due to node-sass not calling its error callback internally, while running node-sass executable triggers the error

maxlath added a commit to inventaire/inventaire-client that referenced this issue Jun 13, 2016
maxlath added a commit to inventaire/inventaire-client that referenced this issue Jun 13, 2016
maxlath added a commit to inventaire/inventaire-client that referenced this issue Jun 13, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

following the removal of working directory-relative import

That is the Sass changelog, not Node Sass.

which I guess is the breaking change mentioned in #1474

It is not. Please take a moment to read the post before spreading misinformation like this:

A breaking change in node-sass made my build break, but apparently without properly rejecting the error.


A simple reduce test case shows the error is produced as expected

// index.scss
@import "no-such-file";
./node_modules/.bin/node-sass index.scss
{
  "formatted": "Error: File to import not found or unreadable: no-such-file\n       Parent style sheet: /Users/michael/tmp/issue-1592/index.scss\n        on line 1 of index.scss\n>> @import \"no-such-file\"\n   ^\n",
  "message": "File to import not found or unreadable: no-such-file\nParent style sheet: /Users/michael/tmp/issue-1592/index.scss",
  "column": 1,
  "line": 1,
  "file": "/Users/michael/tmp/issue-1592/index.scss",
  "status": 1
}

This appears to be an issue with brunch or which ever package is calling node-sass.

@xzyfer xzyfer closed this as completed Jun 14, 2016
@maxlath
Copy link
Author

maxlath commented Jun 14, 2016

@xzyfer my apologies for the mismatch on the breaking changes, I got to say I'm still confused on when the change on working directory import path happened then.

There is still something bothering me here:

  • this test logs the expected error:
var sass = require('node-sass')
var options = {
  file: 'whatever'
}
sass.render(options, function(err, result){
  console.log('err', err)
  console.log('result', result)
})
  • but this one stays silent and keeps eating the CPU
var sass = require('node-sass')
var options = {
  file: 'whatever',
  data: '@import \'../some/inexisting/file\';',
  includePaths: [ '.', 'app' ]
}
sass.render(options, function(err, result){
  console.log('err', err)
  console.log('result', result)
})

It seems it either has to do with the ../ path of the @import or the . in includePaths: if you remove either of those, you get your error message back. Is that still me doing something wrong here?

@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

Thanks for your understanding. Using both data and file is unsupported and
results in defined behaviour. This is likely causing the issue you're
describing.
On 14 Jun 2016 7:22 PM, "maxlath" notifications@github.com wrote:

@xzyfer https://github.com/xzyfer my apologies for the mismatch on the
breaking changes, I got to say I'm still confused on when the change on
working directory import path happened then.

There is still something bothering me here:

  • this test logs the expected error:

var sass = require('node-sass')var options = {
file: 'whatever'
}sass.render(options, function(err, result){
console.log('err', err)
console.log('result', result)
})

  • but this case stays silent and keeps eating the CPU

var sass = require('node-sass')var options = {
file: 'whatever',
data: '@import '../some/inexisting/file';',
includePaths: [ '.', 'app' ]
}sass.render(options, function(err, result){
console.log('err', err)
console.log('result', result)
})

It seems it either has to do with the ../ path of the @import or the . in
includePaths: if you remove either of those, you get your error message
back.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1592 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAjZWKRToHnfBtvI4-gAUyXwNbQeMIRDks5qLnLPgaJpZM4I0H8y
.

@maxlath
Copy link
Author

maxlath commented Jun 14, 2016

Sorry to insiste, but it doesn't look like the issue is that I'm passing both file and data. We can actually remove file and still get a case without any error being returned:

  • this gets the right error
var options = {
  data: '@import \'../some/inexisting/file\';',
  includePaths: [ 'app' ]
}
  • this too
var options = {
  data: '@import \'./some/inexisting/file\';',
  includePaths: [ '.', 'app' ]
}
  • but not this
var options = {
  data: '@import \'../some/inexisting/file\';',
  includePaths: [ '.', 'app' ]
}

@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

Sorry to insiste, but it doesn't look like the issue is that I'm passing both file and data.

Sorry, to be clear, passing both data and file is probably why you're seeing the a different issue

this one stays silent and keeps eating the CPU

@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

I am able to reproduce a process hang with

var sass = require('node-sass')
var options = {
  data: '@import "../does-not-exist";',
  includePaths: ['.']
}
sass.render(options, function(err, result){
  console.log('err', err)
  console.log('result', result)
})

@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

After some digging it looks like the issue is with . being used a load path. Using __dirname or an absolute path instead works as expected.

@xzyfer xzyfer reopened this Jun 14, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Jun 14, 2016

I have confirmed this a bug in LibSass - sass/libsass#2106

@michaelhogg
Copy link

The bug in LibSass was fixed in sass/libsass#2107, and the fix was released in v3.4.0.

The version of LibSass used by node-sass was bumped to 3.4.0 for release v4.0.0.

So I think this issue should be resolved in v4.0.0 onwards.

@nschonni nschonni added this to the 4.0 milestone Dec 28, 2016
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
Fix operator overloading for attribute selectors
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants