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

Always write to response #7

Merged
merged 1 commit into from
Aug 7, 2014
Merged

Always write to response #7

merged 1 commit into from
Aug 7, 2014

Conversation

edwellbrook
Copy link
Contributor

Always write the compiled css to response, but options.response stops the file bring written to disk.

Currently the first request for a stylesheet (before it has been compiled) will 404. On the second run it will use the complied file. My changes mean that the first request will return the compiled css AND write to disk.

For backwards compatibility, using options.response will still write to response, but will not write to disk.

@ilanbiala
Copy link
Contributor

Looks pretty straightforward to me.

@edwellbrook
Copy link
Contributor Author

@keithamus Hi. Any chance you've been able to look at this?

@ilanbiala
Copy link
Contributor

@keithamus if you guys can't look over this package properly and in a timely fashion, why not allow others to contribute to it and add them as collaborators?

@keithamus
Copy link
Member

@ilanbiala there are multiple maintainers for this project, of which I am one. I was a daily user of node-sass but I'm not any more, which means despite having no vested interest in maintaining the project, I still try to find a little free time to help out where I can, mostly to steward the issue tracker, but that is just it - the time is little. In addition it is not my place to say who should and should not be a collaborator on this project.

@edwellbrook Sorry, PRs take a while for me to review due to my lack of free time. I'll take a look when I am next able to, unless another collab beats me to it.

@edwellbrook
Copy link
Contributor Author

@keithamus No worries. Just checking you were aware of it. Thanks, man!

@nschonni
Copy link
Contributor

nschonni commented Aug 6, 2014

There is a bunch of whitespace changes.
Would this do the same for you without duplicating the code

// If response is falsey, write to file
if (!options.response) {
    mkdirp(dirname(cssPath), 0700, function(err){
      if (err) { return error(err); }
      fs.writeFile(cssPath, css, 'utf8', function(err) {
          if (err) return error(err);
      });
    }
}

res.writeHead(200, {
    'Content-Type': 'text/css',
    'Cache-Control': 'max-age=0'
});
return res.end(css);

@edwellbrook
Copy link
Contributor Author

@nschonni great suggestions thanks. made the changes. let me know what you think!

@nschonni
Copy link
Contributor

nschonni commented Aug 7, 2014

LGTM 👍 Do you want to squash the commits down?

@keithamus
Copy link
Member

@edwellbrook you seem to have lost the bit where the code actually does fs.writeFile. Perhaps it got swallowed in the squash?

Always write the compiled CSS to response, with truthy `options.response` also writes the file to disk.
@edwellbrook
Copy link
Contributor Author

@keithamus I think we're good now. Big apologies for those mess-ups

@keithamus
Copy link
Member

Don't sweat it. LGTM 😄

keithamus added a commit that referenced this pull request Aug 7, 2014
@keithamus keithamus merged commit b8ac617 into sass:master Aug 7, 2014
@edwellbrook
Copy link
Contributor Author

@keithamus Thanks man! 😄

@keithamus
Copy link
Member

@edwellbrook thanks for spending your time to make node-sass better!

@edwellbrook edwellbrook deleted the patch-1 branch August 7, 2014 13:24
@edwellbrook
Copy link
Contributor Author

@keithamus Any idea when this'll be updated on NPM?

@keithamus
Copy link
Member

Sorry @edwellbrook, I don't. I don't have the npm publish rights, I believe @andrew does. I'm going to work on getting a travis.yml set up in the next few weeks to autos publish on build.

@ilanbiala
Copy link
Contributor

@keithamus any progress on getting the new commits published in a release on npm?

@keithamus
Copy link
Member

@edwellbrook @ilanbiala sorry not yet. Let me try to get the keys to the castle from @andrew

@edwellbrook
Copy link
Contributor Author

No worries! Thanks :)

@keithamus
Copy link
Member

@edwellbrook Ok, @andrew has kindly given me publish rights - I'm going to set it up tonight so that travis can publish to NPM which will mean any contributors who can tag commits can deploy new NPM versions 😄

@edwellbrook
Copy link
Contributor Author

@keithamus Awesome man! Would be great if you could drop us a message here once it's set up and npm package updated :) Cheers.

@edwellbrook
Copy link
Contributor Author

Hey @keithamus was this ever updated on npm?

@keithamus
Copy link
Member

Waiting for another maintainer to merge #12 - which sets up auto-deploy for this package.

@andrew
Copy link
Contributor

andrew commented Sep 23, 2014

Sorry I missed the boat on that, been avoiding my email a bit for the past week!

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.

5 participants