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

Fail build on compiler error, after printing a useful message #13255

Merged
merged 1 commit into from
Feb 5, 2018
Merged

Fail build on compiler error, after printing a useful message #13255

merged 1 commit into from
Feb 5, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 4, 2018

This PR causes the build to fail after a compiler error, after printing a useful message.

Syntax error:

SyntaxError: amphtml/src/foo.js: Unexpected token (20:6)
  18 | 
  19 | if (foo) {
> 20 |   bar(;
     |       ^
  21 | }
  22 | 

Other errors, like bad imports:

Error: Cannot find module './bar' from 'amphtml/src/foo'

Fixes #1213

@rsimha
Copy link
Contributor Author

rsimha commented Feb 4, 2018

/to @cramforce

@rsimha
Copy link
Contributor Author

rsimha commented Feb 5, 2018

/to @erwinmombay @choumx

console.error(red(err.message));
}
// Drop the node_modules call stack, which begins with ' at'.
const message = err.stack.replace(/ at[^]*/, '').trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we want to drop it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack has nothing to do with the actual syntax error, and gets cumbersome to read. For example, here's what I see when I print the entire stack:

SyntaxError: /usr/local/google/home/rsimha/src/amphtml/src/service-worker/kill.js: Unexpected token (28:6)
  26 | });
  27 | if (foo)
> 28 |   bar(;
     |       ^
  29 | 
  30 | self.addEventListener('activate', function(event) {
  31 |   // Usually after waiting for a navigation, it has to wait for all other
    at Parser.pp$5.raise (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:4454:13)
    at Parser.pp.unexpected (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:1761:8)
    at Parser.pp$3.parseExprAtom (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3750:12)
    at Parser.pp$3.parseExprSubscripts (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3494:19)
    at Parser.pp$3.parseMaybeUnary (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3474:19)
    at Parser.pp$3.parseExprOps (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3404:19)
    at Parser.pp$3.parseMaybeConditional (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3381:19)
    at Parser.pp$3.parseMaybeAssign (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3344:19)
    at Parser.pp$3.parseExprListItem (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:4312:16)
    at Parser.pp$3.parseCallExpressionArguments (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3573:20)
    at Parser.pp$3.parseSubscripts (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3533:31)
    at Parser.pp$3.parseExprSubscripts (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3504:15)
    at Parser.pp$3.parseMaybeUnary (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3474:19)
    at Parser.pp$3.parseExprOps (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3404:19)
    at Parser.pp$3.parseMaybeConditional (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3381:19)
    at Parser.pp$3.parseMaybeAssign (/usr/local/google/home/rsimha/src/amphtml/node_modules/babylon/lib/index.js:3344:19)

@rsimha rsimha merged commit 515e812 into ampproject:master Feb 5, 2018
@rsimha rsimha deleted the 2018-02-03-FailOnBadImport branch February 5, 2018 22:47
@calebcordry
Copy link
Member

@rsimha-amp this error message is awesome. One (unintended?) result is that if I am running gulp watch while developing, a syntax error will kill the watch mode and I will have to manually restart. (it does print the nice error message 👍 ).

Is this the intended behavior?

@rsimha
Copy link
Contributor Author

rsimha commented Feb 6, 2018

@calebcordry Good catch, that wasn't intentional. I think we should exit if there's a syntax error during gulp build or during the initial build phase of gulp or gulp watch, but we should not exit when a file is recompiled during the subsequent watch phase. Would this work?

I'll send you a PR. Edit: Sent out #13312

@calebcordry
Copy link
Member

SGTM. Thanks!

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

Successfully merging this pull request may close these issues.

5 participants