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

Async custom importer sass.NULL behavior different than sync importer #1296

Closed
lingram-rmn opened this issue Dec 5, 2015 · 5 comments
Closed

Comments

@lingram-rmn
Copy link

Returning sass.NULL asynchronously does not result in the default libsass import behavior. It's quite possible that this will turn out to be an upstream bug in libsass, but I thought I would start here.

Repro

Run the following in a context where node-sass is npm-installed:

var assert = require('assert');
var fs = require('fs');
var path = require('path');
var sass = require('node-sass');

fs.writeFileSync(
  path.resolve('entry.scss'),
  [
    '/* entry.scss */',
    '@import "./import.scss";'
  ].join('\n')
);

fs.writeFileSync(
  path.resolve('import.scss'),
  '/* import.scss */'
);

var syncResult;
sass.render({
  file: path.resolve('entry.scss'),
  importer: function (url, prev, done) {
    return sass.NULL;
  }
}, function (err, result) {
  if (err) {
    throw err;
  }

  syncResult = result.css.toString();
});

sass.render({
  file: path.resolve('entry.scss'),
  importer: function (url, prev, done) {
    done(sass.NULL);
  }
}, function (err, result) {
  if (err) {
    throw err;
  }

  setTimeout(function () {
    assert.strictEqual(result.css.toString(), syncResult);
  }, 0);
});

Expected

Script exits without an error.

Observed

The equality assertion fails:

assert.js:86
  throw new assert.AssertionError({
        ^
AssertionError: '/* entry.scss */\n' === '/* entry.scss */\n/* import.scss */\n'
    at null._onTimeout (/Users/lingram/Projects/node-sass-importer-bug/test.js:44:12)
    at Timer.listOnTimeout (timers.js:119:15)
@JohnAlbin
Copy link
Contributor

@lingram-rmn I'm not certain, but is this a duplicate of issue #1291? Or possibly a manifestation of the same problem that the importer's node-sass is not equal to the node-sass that calls the importer?

@lingram-rmn
Copy link
Author

I don't think it's a duplicate, no. There's only one node-sass instance
involved.
On Dec 5, 2015 7:48 PM, "John Albin Wilkins" notifications@github.com
wrote:

@lingram-rmn https://github.com/lingram-rmn I'm not certain, but is
this a duplicate of issue #1291
#1291? Or possibly a
manifestation of the same problem that the importer's node-sass is not
equal to the node-sass that calls the importer?


Reply to this email directly or view it on GitHub
#1296 (comment).

This e-mail, including attachments, contains confidential and/or
proprietary information, and may be used only by the person or entity to
which it is addressed. The reader is hereby notified that any
dissemination, distribution or copying of this e-mail is prohibited. If you
have received this e-mail in error, please notify the sender by replying to
this message and delete this e-mail immediately.

saper added a commit to saper/node-sass that referenced this issue Dec 20, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

Fixes sass#1296
@saper
Copy link
Member

saper commented Dec 20, 2015

I am not very happy about this fix, but can you check if #1308 works? It fixes it for the script above.

saper added a commit to saper/node-sass that referenced this issue Dec 20, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

While here, allow calling done(null) to anticipate
removal of the sass.NULL one day.

Fixes sass#1296
xzyfer pushed a commit to xzyfer/node-sass that referenced this issue Dec 27, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

While here, allow calling done(null) to anticipate
removal of the sass.NULL one day.

Fixes sass#1296
xzyfer pushed a commit to xzyfer/node-sass that referenced this issue Dec 27, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

While here, allow calling done(null) to anticipate
removal of the sass.NULL one day.

Fixes sass#1296
@xzyfer xzyfer added this to the next.patch milestone Dec 27, 2015
@xzyfer
Copy link
Contributor

xzyfer commented Dec 27, 2015

@lingram-rmn we've confirmed this issue. It will be fixed in v3.4.3.

xzyfer pushed a commit to xzyfer/node-sass that referenced this issue Dec 27, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

While here, allow calling done(null) to anticipate
removal of the sass.NULL one day.

Fixes sass#1296
xzyfer pushed a commit to xzyfer/node-sass that referenced this issue Dec 27, 2015
Whenever done() is called we should check for the sass.NULL
value as well.

While here, allow calling done(null) to anticipate
removal of the sass.NULL one day.

Fixes sass#1296
@lingram-rmn
Copy link
Author

👏 Thanks! I'm back in the office on Monday and will test the patch out then. 👏

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