CI Cleanup

[ef4]

• I searched through our entire dependency graph and couldn't find anything consuming DISABLE_SOURCE_MAPS or BROCCOLI_ENV. I think they are entirely vestigial.

• DEBUG_RENDER_TREE was being set in an irrelevant place, because it doesn't take effect during the build, it takes effect during the test run. But when I tried to enable it there, I found that it never has any effect anyway. The issue is that DEBUG_RENDER_TREE is always enabled in dev mode, and even when the test suite is built in prod mode it still loads ember-template-compiler.js, which always includes dev-mode code that forces DEBUG_RENDER_TREE to on. So there's no way to run the test suite with DEBUG_RENDER_TREE off, so there's no need to have an extra test that tries to turn it on.

• edition=classic: we dropped this at ember 4, but we're still doing extra test runs for it.

[chancancode]

DEBUG_RENDER_TREE was being set in an irrelevant place, because it doesn't take effect during the build, it takes effect during the test run. But when I tried to enable it there, I found that it never has any effect anyway. The issue is that DEBUG_RENDER_TREE is always enabled in dev mode, and even when the test suite is built in prod mode it still loads ember-template-compiler.js, which always includes dev-mode code that forces DEBUG_RENDER_TREE to on. So there's no way to run the test suite with DEBUG_RENDER_TREE off, so there's no need to have an extra test that tries to turn it on.

Something probably broke/got lost along the way then. The point of DEBUG_RENDER_TREE is to support the inspector when the inspector is present, but otherwise is commonly expected to be disabled in production for the ordinary user. So the intent is to make sure all of these combinations work:

• Debug
• Prod without ENV._DEBUG_RENDER_TREE (your typical site visitor)
• Prod with ENV._DEBUG_RENDER_TREE (Ember developer visiting the production app with the inspector loaded)

The reason it has to be tested separately is the flag is the be set before Ember is loaded and it gets consumed by module level code and cannot be unset (or set later)

[ef4]

Yes, debug and prod-with-inspector were already being tested and are still being tested.

Prod-without-inspector was never actually being tested because of the way the test suite works. It can run against the ember bundle built for prod mode. But it also always loads the template compiler bundle, and that is always built in dev mode, and it always mutates EmberENV to enable debug-render-tree.

I am planning to implement the improved API for runtime template compilation from RFC 931, which will let us stop loading the older template-compiler bundle and all its quirks into the test suite. I think that would be a reasonable way to implement coverage for prod-without-inspector.

[chancancode]

I understand that it is currently/already broken on main today, and likely have been broken from quite some time (I think @kategengler noticed it and mentioned to me a while back).

However, I am not sure about that it never worked. For example:

ember.js/packages/@ember/-internals/glimmer/tests/unit/template-factory-test.js

Line 71 in a435903

templateCacheHits: ENV._DEBUG_RENDER_TREE ? 5 : 2,

I think it would have been difficult/unusual for someone to notice these kind of differences and went out of their way to fix them if the tests never worked correctly (who runs prod tests locally?), and I am pretty sure it was working when I originally added it.

Anyway, as long as it is understood that the scenario is definitely important to test (essentially we are not testing the most common production scenario right now), and as long as that is tracked (we should probably open an issue?) and there is a plan to restore it soon-ish, that seems good to me.

[chancancode]

Actually, based on your description of the problem I think there can be a relatively easy fix without waiting for the other refactor to be complete.

I think something about how the scripts-loading works have probably changed, which is not surprising given the build refactors.

In tests/index.html we do load ember.debug.js/ember.js before loading the template compiler. It was probably the case that at one point, that (or the few lines after but before loading the template compiler) was sufficient to ensure the env.ts is evaluated and pinned/cached in loader.js, fixing the value of ENV._DEBUG_RENDER_TREE before the template compiler is loaded. By the time the template compiler is loaded, loader.js probably just ignores the subsequent define() for the same module. Or perhaps somehow the two scripts were previously using separate instances of loader.js and they now share it after some refactor.

It seems like something must have changed to cause the modules from the template compiler bundler to sometimes stomp over the ember.prod.js modules, which is certainly problematic and must have other consequences beyond this (that leads to decreased/invalid production test coverage), so fixing the fundamental conflict certainly seem important.

For this particular case though, we can probably force a require to that module before loading the template compiler as a quick fix, which seems worth doing for now to regain the pretty important test coverage that we apparently lost for some time.

[chancancode]

Ah, my educated guess would be that this broke when we switch from ember having its internal loader to being "real modules" in the global/shared loader. It was probably the case that those internal modules were kept separate (ember.prod.js and ember-template-compiler.js each having its own "internal loader") prior to that change.

That seems like it would have real world implications/consequences to apps as well – essentially if you load the template compiler in production, it have some chance of stomping over some of the ember modules and replacing them with the debug version of the same module.. so you're running a mix of production and debug modules.. which certainly.. can't be good?

I suppose too few people actually use the runtime compiler, and the chance of things going wrong this way are slim, and when it does, probably nobody could figure out what is happening 🤷🏼

[ef4]

Thanks for investigating. I will open an issue to track the lack of test coverage before merging this.

[ef4]

I'll add that there's no reason we couldn't build the template compiler bundle with the prevailing DEBUG mode, we just don't, and I have been hesitant to make changes because I'd rather keep it as stable as possible and then migrate people off it entirely when the new API ships.

[kategengler]

This defaults to false https://github.com/emberjs/ember.js/blob/main/packages/%40ember/-internals/environment/lib/env.ts#L97

I suspect the others also default to their "classic" values. There are other tests that set the optional flags one way or another and probably what we need to do is remove the optional features we no longer support, but at the very least, our tests should keep the octane version of tests.

[ef4]

Good catch. I'm pretty sure it's impossible for a real app to set these to the non-octane values because of the assertions in lib/index.js. So we can probably just rip these flags out and hard-code the octane behavior.

[kategengler]

Yeah I think it is just missed cleanup

[kategengler]

Actually looks like there's no assertion on async observers

[ef4]

We should deprecate synchronous observers. Whether by 6 or by 7 I don't have a strong opinion, it would depend on how prevalent the community dependence still is.

[ef4]

I'm going to separately land cleanup for the octane-mandatory optional features and then rebase this PR.

#20683

[ef4]

#20685

[ef4]

Ok, so I've now separately removed the unreachable optional features post-4.0.

As Katie pointed out, the default-async-observers is still a legitimate option. In terms of how we test it, it doesn't need separate test runs with the global flag in different states, because the tests ask explicitly for async vs sync observers where they care, and the flag only changes the default for that argument. No test does anything different under the different flag values.

[ef4]

The expected-but-incomplete test that CI is showing is removed in this PR and I removed it from the GitHub configuration but existing PRs don't reflect that.

[ef4]

I'm tracking the DEBUG_RENDER_TREE tests in emberjs/rfcs#933 because that is tracking the work for the new runtime template compiler API.