Move forward with warn-as-error by default

I would be happy to hear if you have a better solution for reliable editor error reporting

Fully agree with @jfrohlich.

During development

  • Build should pass even if you have unused variables
  • Editor should show the unused variables though if possible (works with the current RLS AFAIK?)
1 Like

At Ahrefs, we use "warnings": {"error": "+A-d"}, by default in all the frontend codebase, so what Bob proposes is aligned with our setup. This configuration forces us to go to some extra lengths sometimes to remove warnings from generated code (e.g. https://github.com/ahrefs/atd/pull/214) but we think itā€™s worth it. In our experience, it is much cheaper to fix a warning in context, while one is editing the file, rather than waiting for CI to report the error way later in time.

Itā€™s great to also leave a work around / escape hatch if needed.

I would be happy to hear if you have a better solution for reliable editor error reporting

@Hongbo if the editor extension somehow called bsc.exe on the currently edited file individually, then warnings would not be lost between runs, right? (I understand this would involve having the editor somehow reading from build.ninjaā€¦ but just curious if itā€™d be a viable approach).

3 Likes

Weā€™re exploring something to improve the user experience in warn-as-error, e.g, keep the build going as far as it can to list all errors in one build.

Only the build system has a global view of the repo to provide reliable error diagnostics

1 Like

Unless I misunderstoodā€“the compiler wouldnā€™t enforce anything different for existing codebases. We are talking about warning and error settings for new codebases only, i.e. changing the recommended bsb -init templates. But maybe @Hongbo can confirm.

1 Like

I will write a post about the new design, good news is that we can achieve the best of bost worlds when we own both the compiler and build system

2 Likes

After some hacks, I think we can still generate js files even with non-critical errors, however, we need fake the time stamp of generated js files to notify itā€™s dirty. (it works on my OS, need test Windows)
In this case, would it be better?

11 Likes

This is excellent news, thanks a lot for your efforts! :smiley:

I am not sure what you mean by faking the timestamps though, could you explain that in more detail?

Let me know if you would like me to test someting! :slightly_smiling_face:

1 Like

Amazing, Bob. Thank you!

I didnā€™t have time to respond to the announcement thread yesterday, I can understand why errors are better from a build system perspective but I definitely rely on continuing to generate JS with non-critical errors. I will put up with any hacks required to do so and I support this move.

A future IDE improvement could even flag non-critical errors as warnings in the IDE :wink:

1 Like

we set mtime of generated js file to be 0 and change the build system to think it is non-exist. Will publish a dev release for testing

1 Like

Awesome @Hongbo! This would really be best of both worlds.

bs-platform@8.3.0-dev.2 is available for testing
The obvious difference (desired behavior):

  • projects built which has warnings will have an exit code which is not 0
  • files with warnings will always trigger rebuild

The non-obvious difference:

  • generated js files which come from modules with warnings will have a faked old timestamp
  • If you have a js_post_build_command (ignore if you donā€™t know about it), it will not be triggered when the module has warnings
12 Likes

Sounds like the perfect compromise. I also like the non-zero exit code, to fail the CI when there are warnings, but still have a good dev experience.

I have to say Iā€™m really impressed by the work youā€™ve done @Hongbo, managing to get the best experience for both use cases. It must be really hard to satisfy everybody and listening to feedback must sometimes feel overwhelming but that leads to great solutions, thank you a lot for taking the time! We not only have the best alt-js language, we also have a great community and itā€™s really cool to be able to benefit from it :smiley:

8 Likes

Just tested it on a large code base (Iā€™m on OSX): The hack works like a charm. An error is reported but the file is built. Gotta love the wizardry :clap:t2:

4 Likes

Just tested it, too, and it worked perfectly. This is a huge relief! Thanks again for your awesome work @Hongbo! :smiley: :+1:

2 Likes

Will there be a proper solution for this ā€œhackā€? Faking the timestamp is interacting badly with the metro build system of react-native (probably it tracks if the timestamp to see if the file has been updated), so need to resolve all warnings first before a good fast-refresh is happening.

is the warn as error (but trigger build for non critical errors) default behavior in latest version? if not how can I enable it?

You can set:

"warnings" : { "error" : true}

in you config.

Note we introudced a more advanced approach so that warnigns are always reproducible without enforcing warn-as-error

2 Likes