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

address hanging process on SASS syntax error #5

Closed
wants to merge 1 commit into from
Closed

address hanging process on SASS syntax error #5

wants to merge 1 commit into from

Conversation

esc-mhild
Copy link

Problem: A syntactic error in a SASS file may lead to a hanging process due to a known issue in node-sass (sass/node-sass#1048).

Solution: Invoke sass.renderSync instead of sass.render.

Discussion:

  1. The issue occurs, for instance, when running the plugin within grunt-postcss (e.g. when compiling test/sass-error.css in this PR). Any SASS syntax error appears to lead to a hanging process.
  2. The PR adds a test case to verify a clean exit on encountering a SASS syntax error (sass-error.css). As part of the tape test suite, this test succeeds with the original postcss-sass code. I am not sure how to explain this fact; the internal logic of a tape test in npm may account for it.
  3. The suggested solution in node process hangs on unrelated error when using custom importers sass/node-sass#1048 does not appear to address the issue when running in grunt.
  4. My proposal is to invoke sass.renderSync and catch any error it may emit.
  5. As a side-effect of invoking sass.renderSync instead of sass.render, the existing test of import functionality (test/imports.css) now triggers the (appropriate) deprecation warning "Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass" (see, for instance, Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass. sass/node-sass#2362). (This message could be suppressed by adding the .css extension to @import statements in test/import.css, but then the test imports:postcss would fail.)

Jonathan, thank you for your work on this plugin!

Best wishes,

M.

@jonathantneal
Copy link
Collaborator

Is this still an issue in Sass? This is a very clever work around that I’m not entirely opposed to adding, but I’d prefer it were resolved on Sass’ side, of course.

@esc-mhild
Copy link
Author

Your 3.0.0 commit (with many version updates) seems to take care of the issue.

Many thanks!

@esc-mhild esc-mhild closed this Dec 10, 2018
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.

2 participants