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 error reporting of lessc executable II #2891

Merged
merged 1 commit into from
May 9, 2016

Conversation

jhnns
Copy link
Contributor

@jhnns jhnns commented May 3, 2016

This commit replaces the old control flow of exiting the process when an error occurred which swallowed the error in some situations (#2881). It also adds process.exitCode = 1 in some error situations that have previously been reported as exitCode = 0. Additionally, it adds a listener for "unhandledRejection" to also catch errors caused by rejected promises.

This pull-request incorporates #2882

If you don't want to support node 0.10.x anyway, I can also push a simplified version. Then we probably also should remove the engine property from the package.json.

This commit replaces the old control flow of exiting the process when an error occurred which swallowed the error in some situations (less#2881). It also adds process.exitCode = 1 in some error situations that have previously been reported as exitCode = 0. Additionally, it adds a listener for "unhandledRejection" to also catch errors caused by rejected promises.
@jhnns jhnns mentioned this pull request May 3, 2016
@matthew-dean
Copy link
Member

If you don't want to support node 0.10.x anyway

That seems like a pretty old version to me. In my mind, it's safe to remove 0.10.x support if it simplifies the code.

@matthew-dean
Copy link
Member

@less/core Fixing Node 6 support seems like an important PR to get out soon, so can I get some other opinions on this?

@jhnns
Copy link
Contributor Author

jhnns commented May 3, 2016

Fixing Node 6 support seems like an important PR to get out soon

This PR is not actually fixing anything for Node 6. As discussed at #2881, the main problem is already fixed on the master branch. Another bug that occurs in combination with the --source-map flag is still present and requires fixing.

This PR fixes the problem that these bugs were even not reported to the user.

@jhnns
Copy link
Contributor Author

jhnns commented May 3, 2016

That seems like a pretty old version to me. In my mind, it's safe to remove 0.10.x support if it simplifies the code.

I agree. However, it's still included in the engine field of the package.json. Travis CI is also running node 0.10.x tests.

@matthew-dean
Copy link
Member

matthew-dean commented May 10, 2016

@jhnns I had a look at Node releases today. The latest 0.10.x release was 4 days ago, same as 0.12. It must be because of their LTS. So I don't think it's safe to remove 0.10.x support. I changed it in the engine field to >=0.12 but we should put support back in for 0.10, and make sure it's passing Travis.

Travis I think still has an outstanding issue where it's not able to install / run PhantomJS sometimes. So you get PR fails when the code is fine, so it's been unfortunately unreliable lately. I tried to tweak settings to get it to work, but it's not my area of expertise.

@jhnns
Copy link
Contributor Author

jhnns commented May 11, 2016

I did not remove node 0.10 support. This should work (although I haven't tested it).

Regarding Travis: Have you tried to increase the test timeout?

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