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: middleware can't deal with more than one source file correctly #61

Merged
merged 1 commit into from
Oct 21, 2015

Conversation

laino
Copy link
Contributor

@laino laino commented Oct 5, 2015

When rendering the first input file options.outFile is set once and later re-used
for every subsequent file.

This caused two problems:

  1. When rendering fileA.sass and fileB.sass in that order,
    the result of fileB.sass would overwrite fileA.css afaik.

I never really got that far since:

  1. node-sass saves the callback in the supplied options object, overwriting the previous callback.
    When two .css were requested in parallel, this caused the callback of the first rendering
    process to be called twice and the callback of the second rendering process to be overwritten and
    hence never called. Which THEN caused done (our callback) to be called twice, leading to
    Error: Can't set headers after they are sent.

Here's the specific problematic line:

options.outFile = options.outFile || cssPath;

As you can see we set outFile once and then re-use it for every subsequent rendering process.
We also pass our initial options object to node-sass, which will save some own data in it causing problematic behavior when rendering more than one file at once.

This PR fixes that.

@am11
Copy link
Collaborator

am11 commented Oct 10, 2015

Thank you much for investigating and addressing this issue @laino.

Although the cloning approach will fix your scenario and #62, this is not the intended behavior of the node-sass feature under question, which exercises the mutability of options object; alters/updates and adds additional properties to the same in the post-compilation phase, in order to provide the downstream with 'resolved' paths, additional stats etc. (further tends to conform with the ruby-sass behavior).

In order to propagate this behavior transparently to the nth downstream of node-sass, middleware need to make sure for every compilation request, render() is not being called multiple times. Note that although it is not required in middleware scenario, but it would be nice to keep the implementation consistent with the said node-sass feature.

Nonetheless, if you can provide a self-contained example which would demonstrate how the middleware is misbehaving and expected outcome, we can probably ideate the best solution here. This is something we can think about later as well, meanwhile this PR LGTM modulo it can use a unit test. :)

@am11 am11 mentioned this pull request Oct 10, 2015
@scottbrady
Copy link

I'm running into the same bug after upgrading from 0.5.0 to 0.9.6. I have multiple CSS files included on a single page in my application. If the middleware is processing more than one CSS file at the same time the Node process crashes with the stack trace:

Error: Can't set headers after they are sent.
  at ServerResponse.OutgoingMessage.setHeader (http.js:690:11)
  at ServerResponse.setWriteHeadHeaders (node_modules/compression/node_modules/on-headers/index.js:82:19)
  at ServerResponse.writeHead (node_modules/compression/node_modules/on-headers/index.js:41:36)
  at doneWriting (node_modules/node-sass-middleware/middleware.js:171:13)
  at node_modules/node-sass-middleware/middleware.js:197:11
  at node_modules/browserify-middleware/node_modules/watchify/node_modules/chokidar/node_modules/readdirp/node_modules/graceful-fs/graceful-fs.js:42:10
  at Object.oncomplete (fs.js:108:15)

am11 added a commit that referenced this pull request Oct 21, 2015
Fix: middleware can't deal with more than one source file correctly
@am11 am11 merged commit fed4fd4 into sass:master Oct 21, 2015
@am11
Copy link
Collaborator

am11 commented Oct 21, 2015

Thanks @scottbrady for chipping in. I will make a release for this hotfix and later will redo it in a better way, if there is any.:)

Also copying @jakobo, (the original author of this feature in upstream): would you suggest doing something different here, in order to prevent downstreams cloning the options object (esp. in this multiple web-requests scenario)? Thanks.

@am11
Copy link
Collaborator

am11 commented Oct 21, 2015

v0.9.7 is released with the fix.

@scottbrady
Copy link

Looks like 0.9.7 fixes the crashing issue we were experiencing. Thanks for quickly taking care of this bug.

Another option to fix this problem would be to use the synchronous render method:

try {
  var result = style.renderSync(options);
  done(null, result);
} catch (e) {
  done(e);
}

We only use node-sass-middleware in development mode (to make it easier for devs) so using a sync method would be fine for us. We don't use this middleware in production; instead we compile the CSS during deploy and serve the static files. I'm not sure what use cases other people might have.

Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants