Move WAITER_NAMES to global context to reduce warning log spew when this addon becomes v2 and duplicates are allowed

[NullVoxPopuli]

NOTE: this PR will need to target to the main branch as well (which I plan on doing after all the test apps are implemented)

Context on "The Plan": #458

historically, we've only been able to allow one copy of test-waiters in a whole projcet.
this was forced via build-time code in the v1-addon's index.js.
This added maintenance complexity and prevented the conversion to a v2 addon.

With this runtime injection of global state, we can have both the pre-v2 addon
and the v2 addon share the same waiter state.
Also, in @ember/test-waiters@v3, we can force the existing build-time code to
take precedecence with the latestVersion of test-waiters v3 (existing behavior),
so there is no risk of old copies of test-waiters not using this runtime highlander code,
because those older copies would not be present in the final build of an app.

---

So what does this PR actually do?

• Moves all module state to globalThis
  • getGlobal already existed, so much of this work was already done
  • the WAITERS have already been handled
  • this PR focuses on moving WAITER_NAMES to also be on the global context, so we don't create a ton of log spew.
• More specific types to its clearer what the shape of data is for each of these structures
  e.g.:

  globalThis: {
    // existing
    [Symbol<'TEST_WAITERS'>]?: Map<WaiterName, Waiter> | undefined;
    // new!
    [Symbol<'TEST_WAITER_NAMES'>]?: Set<string> | undefined;
  }

---

NOTES

• declared compatibility of the 3.x series of this addon include Ember 3.8, which includes IE11 -- so we can't go full native Symbol support yet.

[NullVoxPopuli]

the core of this PR is moving WAITER_NAMES from module-state to state that lives on globalThis so that it can be shared

[chancancode]

It seems like we don't really need this at all. It appears that the purpose of this is just to keep track of what names has been already been used and issue a warning. But in all cases, immediately the waiter is registered on the Map with that string anyway, so it seems like we could have just checked the map for an existing entry in register?

[NullVoxPopuli]

yeah it does seem that way -- I was trying to not change how things work too much -- but maybe everything gets way simpler when checking the map. I'll push an update with that, removing WAITER_NAMES, so the already global WAITERS is the source of truth.

This would make this PR optional, and it would mean that we can probably proceed with v2-ification without this PR.

The changes coming (not yet pushed) will make the implementation simpler tho, and simpler is still a good thing to have going forward.

[NullVoxPopuli]

nope, this is a false assumption, it seems.

The waiter names are managed like this so you log a warning once per name.

And we def want that to be retained across the globalThis boundary, so the code stays.

[chancancode]

Is this theoretical/hypothetical, or were you actually observing the log spew in tests or something? If it's in tests, I suspect it may be more of an artifact of the test environment than a reflection of reality.

Or maybe I wasn't precise enough when I said do it in register. I meant the register in waiter-manager.ts.

buildWaiter isn't the kind of thing you intentionally call over and over again. You'd just do it once somewhere in a static (module scope) context. It is possible that it could be called more than once, which is why this warning is here, but it would be due to some accidental module graph duplication or some addon developer programming error (accidentally reusing the same string in multiple files).

But, it seems quite unlikely to me that it will be called repeatedly with the same string so often that we'd have to worry about log spew? I mean, you can force it to happen, by calling it inside a loop, I suppose. But anytime that it is called more than once is already an error situation, and repeatedly over and over again with the same exact string would be some kind of outrageous error situation, and perhaps it isn't even a bad idea to surface it that way if it somehow happens.

So it doesn't seem like a big deal to me to forgo that kind of "protection", and looking at where this was originally introduced (#126), I don't see any reference to the intention of preventing log spews either.

[NullVoxPopuli]

Is this theoretical/hypothetical, or were you actually observing the log spew in tests or something?

This is preparing for switching the library to a v2 addon (See "The Plan"), where we'll have no more highlander code.

Today, with the v1 addon, the WAITER_NAMES stuff is a non-issue because there is only one copy of @ember/test-waiters -- that will likely not be the case with native v2 addons.

As for hypothetical (due to this library not being v2 yet) -- we're saying that "all waiters, across all parts of the module graph, must have unique names".

[chancancode]

That would be an issue even before this PR tho

Yep it is a bug

This is preparing for switching the library to a v2 addon (See #458), where we'll have no more highlander code.

Right but I am specifically talking about the part where you were worried about log spew.

As far as run time behavior and correctness, the global Map already forces the waiters to have unique names or else they would stomp over each other right now. This is true today and also true in the v2 world.

This code we are talking about here only exists to provide a warning about that scenario. What I was proposing is that to tie the warning to the underlying global map and avoid having to track the names separately, which is more code and has the issue I pointed out above where things get out of sync.

In response to that, you mentioned that the way this code is written today would prevent log spews.

I agree that is something that the current code happens to do, but I am not sure it was added specifically for that purpose, or that there are real world scenarios where it would be important to prevent log spews with this API.

[NullVoxPopuli]

but I am not sure it was added specifically for that purpose, or that there are real world scenarios where it would be important to prevent log spews with this API.

So you want log spew in the event there is a naming collision, by means of only checking the Map's keys, and always warning?

What would give you certainty about... anything. I feel like we're not getting anywhere

[chancancode]

Ok I was hedging a little bit there and maybe that phrasing confused you. I am saying I didn’t see any evidence that this code is specifically trying to avoid log spew even going back to the original commit. So I am asking where did that concern come from.

Can you read my comment again? I was specifically saying/clarify “it doesn’t seem like we would have to worry about log spew here based on the usage patterns, is there a specific reason you are concerned?”

I was quite specific about why I don’t think that is an issue here and why I didn’t think this was a specific concern when the code was originally added.

I could definitely be wrong but I am asking where did the concern of log spew came from - was it from deductions from analyzing the code, or from discovering the problem when running the tests, etc.

[NullVoxPopuli]

“it doesn’t seem like we would have to worry about log spew here based on the usage patterns, is there a specific reason you are concerned?”

It can happen with a dep graph with duplicate dependencies which would call buildwaiter multiple times.

why I didn’t think this was a specific concern

the highlander code took care of it though, which is why it wasn't a concern.

but I am asking where did the concern of log spew came from - was it from deductions from analyzing the code, or from discovering the problem when running the tests, etc.

the concern comes from

• with native packages, we will have situations where a dep graph contains duplicate dependencies
• with duplicate dependencies, we'll have multiple waiters trying to use the same strings
• if we were to handle the logging in the register of waiter-manager.ts, then for each duplicate dependency, we will log extra -- for each duplicate.

I could take the argument that we want to be noisy about this situation and then we should log extra... but I want to be sure that it's a conscious decision.

(In which case, I can move the waiter name checking to use the waiter map's keys, as you've suggested)

[NullVoxPopuli]

Gonna close this PR because the effort is not worth the benefit.

The changes we want are going to be in the next major anyway, so if folks want the upcoming benefits, they can upgrade and to an overrides/resolution to get the benefit.

The main thing here:

• the list of waiter names is checked eagerly, yet waiters are registered lazily
• the warning is eagerly logged
• we want the map of waiters to be the source of truth

[chancancode]

Yeah we discussed on Discord –

• The critical aspect here, which was the bulk of the work in the original PR, is to ensure the actual waiters map is shared. turns out this is already the case, and we can maintain compatibility with the existing version without requiring forcing resolution, etc, which is nice.
• With that out of the way, the secondary thing that this is trying to move off of module state, is the WAITERS_NAME set:
  • The purpose of this set is to issue a warning whenever a conflict arise – typically this is either a coding error by an addon author or a module is unexpectedly duplicated. Either way it is a error situation and things will be quite broken. It's not entirely clear to me why it is a warning rather than an error, and it's not very clear to me how much things are expected to degrade "gracefully" in these situations.
  • The reason the conflicting name is a problem is that we store the waiters in a map, using the name as the key, inherently that means there can only be one thing with the same name. Since this is ultimately the reason we care, the natural thing to do would be to warn (or throw, even) when trying to register a second instance with the same name, which would also remove the need for a parallel data structure that tracks the same thing (and we don't currently do a great job keeping the two in-sync – we don't remove the names from the set when unregistering, for example, which probably means the unregister API is unusably broken if you intend to re-use the same name).
  • In the current code, moving the warning to the registration side would defer the warning, since register isn't called eagerly in buildWaiter. It would be slightly sub-optimal and make things slightly more difficult to debug when the problem does happen (the problem = a package consuming ember-test-waiters is accidentally duplicated). Personally, I think that is probably acceptable.
  • But it is also not clear that we have a particular good reason to defer adding the waiter to the map in the first place, we can probably just try to move the register call out to buildWaiter (or in the constructor of the waiter) and see if anything breaks, and solve both problems at once.
  • Overall, I think all of these are fine to change, especially since I consider this warning only advisory in nature. However, there are som slight concerns over doing these kind of changes in a minor release, and since we are going to cut a new major imminently anyway, we may as well do it then.
  • If there is a mix of the new legacy version of ember-test-waiters and the new version in the system, we may not always successfully issue this warning. It is probably acceptable since the warning is advisory, but if we want to, we can also try to address this compatibly in the new major by putting a wrapped Map on the global that warn/throw when on the .set method when a conflict is detected.