rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
152 stars 47 forks source link

Compiler never rejects the returned promise #320

Closed luis-soares-sky closed 1 day ago

luis-soares-sky commented 3 years ago

ProgramBuilder.run() is currently an async method that doesn't return any value, regardless if the build is successful or not. This prevents us from easily interrupting our build process when Brighterscript fails to transpile.

Given this example gulp task:

const { ProgramBuilder } = require("brighterscript");
const { LogLevel } = require("brighterscript/dist/Logger");

module.exports = function (gulp, plugins) {
    return (cb) => {
        const builder = new ProgramBuilder();
        builder
            .run({
                extends: "bsconfig.json",
                stagingFolderPath: "build",
                logLevel: LogLevel.log,
                sourceMap: !(global.args.environment == "PROD"),
                createPackage: false,
                watch: false
            })
            .then((result) => {
                // ^ result is `undefined`
                cb();
            })
            .catch((e) => {
                // `catch` only fires in case of non-diagnostic config/plugin load failure
                // or fatal error while executing _runOnce(...)
                cb(e);
            });
    };
};

Because the builder will never return a value even when there are errors, the promise will fire all resolve handlers without any result, and in our case, gulp will keep chugging on. This results in a broken build where most of the files will be missing.

A possible solution would be to return the result of runOnce(), or to throw when the number of errors is != 0, but I'm not sure how it would work with watch mode...

TwitchBronBron commented 3 years ago

Hmm. So, I'm not sure a rejected promise is the right answer here, and I'm not sure what result would be. In your .then() function, you can easily check


if(builder.program.getDiagnostics().filter(x=> x.severity=== DiagnosticSeverity.Error).length > 0){
    throw new Error("Encountered error diagnostics");
};```

What if you wanted to fail for warnings too, but our default is to only fail on errors? 
luis-soares-sky commented 3 years ago

If the builder fails to output any BRS or XML files due to errors, which could be considered invalid output, then I'd argue that the promise should be rejected.

What if you wanted to fail for warnings too, but our default is to only fail on errors?

In an ideal world, it could be configured. Some pseudo-code:

enum FailureLevel {
    None, // default
    OnErrors,
    OnWarnings
}

builder.run({
    failThreshold: FailureLevel.OnWarnings
})
luis-soares-sky commented 1 day ago

Already using the solution proposed by Bronley which is enough for our purposes.