1217503 - Set DEBUG_JS_MODULES on in different environments

Status: NEW

(DevTools :: General, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

What environments should we have this on? Nightly? Local builds? Tests?

This will enable react-dev, which has a performance overhead, so not enabling in nightly is understandable, but then we lose our assertions. And we should be trying to get feedback as early as possible on possible breakage (that's why nightly exists).

Maybe react-dev loading shouldn't be based on this var, but just for local/test builds, whereas DEBUG_JS_MODULES is on in those environments, plus nightly -- can even add telemetry to these assertions failures so we know about it, or in the error provide a link/message to encourage them to report the assertion failure to us.

Comment 1

[James Long (:jlongster)]

gps will have opinions about this. See https://bugzilla.mozilla.org/show_bug.cgi?id=1181646#c24

How would you detect local/test builds? That was actually the original point of DEBUG_JS_MODULES. It was never really meant to be turned on in a live product.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:nalexander and others have used !MOZILLA_OFFICIAL as a proxy for "this is a local build".  DevTools also uses it to clear caches in the Browser Toolbox.  I believe it's not precisely correct since it would also catch people building things like IceWeasel for Linux, since that is also !MOZILLA_OFFICIAL, but it's not a local Gecko developer build.

I don't think there's an explicit "this is a local build" flag that really means only that currently, but maybe there should be?

Another approach is to "default" a setting to the mode you want for local development, but then adjust all mozconfigs that build release versions in the tree to some other value for that setting.

Comment 3

[James Long (:jlongster)]

I still think this is exactly what DEBUG_JS_MODULES was for... It wasn't supposed to be enabled in nightly. It flags that you want this. We can easily turn it on in tests too then.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

So might be better to list some goals, independent of a build flag:

* assertions in JS
* React validations
* no on-by-default performance degradation

Having DEBUG_JS_MODULES enable these things works I think, since it's optional locally, and we can set that build flag for try runs, and encourage developers to use this flag.

Since we can't ship it in nightly due to perf degradation in react (and any other future things that we add under this flag -- we shouldn't worry about perf overhead for this flag in this case), maybe we can have our DTU.assert just log a warning if DEBUG_JS_MODULES is not on, rather than throw, so nightly users still can see/report assertion failures.

Comment 5

[James Long (:jlongster)]

I think a dev-mode implies perf degradation; the other side is that I don't want to worry about perf degradation in my debug code. If we ever end up using webpack or browserify, we could potentially even strip out asserts so they aren't even in production code. That's what a lot of people do. That way you could even do some heavy assertions (regexp matching, etc) and not care about the slight perf hit.

We'll already have a great testing story from nightly users; they'll be the ones willing to send us their actions & state that we can just load up locally. I can see the benefit of having assertions on there, though. But I guess not enough benefit to worry about it too much. If something goes wrong, reproducing it locally is more important than getting them to tell us the failed assertion, and we're getting much better at that with redux & react.

Comment 6

[James Long (:jlongster)]

Another option is to make it an about:config flag that users could turn on if they are trying to reproduce something for us. That has all kind of implications though (it's a build flag right now because we conditionally bundle in react-dev etc)

I dunno, I'm not too opinionated about this, but gps and others in the thread I linked to before seemed to be. I like having one single flag that indicates JS dev mode though.