[BUG] Module/connection version info seemingly(?) not being migrated correctly, resulting in "Missing version information" warnings

[jswalden]

Is this a bug in companion itself or a module?

• I believe this to be a bug in companion

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

I have four modules in module-local-dev/ in my build (presently on main), and in my configuration seven connections using them. Despite evident efforts to avoid this, they trigger a "Some modules are missing version information." box and button, with count of seven missed-info connections.

companion/webui/src/Connections/ConnectionList.tsx

Line 497 in 8f2971a

if (module.devVersion && connection.moduleVersionId === 'dev') continue

attempts to exclude these variables, but the attempt doesn't work. I've added logging into my build, and I see that when it reaches the linked line, module.devVersion is non-null as expected (I assume due to

companion/companion/lib/Instance/ModuleInfo.ts

Line 55 in 8f2971a

devVersion: translateStableVersion(this.devModule),

per cursory code inspection)...but connection.moduleVersionId despite being typed as string actually has value undefined, so the comparison fails. So somehow a badly-typed value is getting into the relevant value store.

I poked more, and it appears this is the result of some sort of inadequate module info migration. I tracked down

companion/companion/lib/Instance/ConnectionConfigStore.ts

Line 112 in 8f2971a

moduleVersionId: config.moduleVersionId,

as having config.moduleVersionId === undefined but couldn't track down where that came from. (Possibly from config data saved to disk?) And when I added a new connection using one of these modules, selecting its "Dev" version, suddenly all the connections' moduleVersionId seem to get rewritten to null and they start falling into this path -- so maybe the "config data saved to disk" hypothesis has now come into play (and updating the store causes actually-typed config data to be written out/read back in). But at that point the warning started to occur (I think a number of times equal to all connections I have, not merely the seven that were from module-local-dev/ but also ones that use bundled modules?) because of this line instead

companion/webui/src/Connections/ConnectionList.tsx

Line 485 in 8f2971a

if (connection.moduleVersionId === null) {

...and at that point my willingness to dive into this maze of code ended. 🙂 (Wild guess: maybe preexisting connections are not being correctly imputed modules on migration? 🤷)

To sum up: there seems to be something wrong in all this. And hopefully some of the flow of consciousness above is helpful in understanding and fixing it. (And loud apologies in advance for whatever portion of it isn't!)

Steps To Reproduce

No response

Expected Behavior

No response

Environment (please complete the following information)

Additional context

No response

[Julusian]

This fix a8e3c69 will probably help you a bit

It sounds as though we are maybe not filling in the missing moduleVersionId on each module correctly at first launch. null is an expected state, waiting to be resolved to a real value as soon as the system can, but I guess undefined isn't.

This isn't going to be the smoothest conversion, because we don't bundle any modules, so we can't fill in exact versions until things are installed and we arent auto-triggering the install of missing modules.

When I updated my staging system the other day I had to click the 'install missing versions' button and it installed almost everything (some since fixed bugs stopped it from finding everything). I don't have any dev modules on that system though

[Julusian]

I've made another fix for this.

It wasn't correctly fixing up some version numbers, notably filling in 'dev' when a dev version was on disk, and maybe nothing at all if the first try didn't find a suitable version.

Could you have another play and see if any cases are still missing?

[jswalden]

Soooooooo...

I absolutely can't say that I'm testing this correctly!

What I have done to attempt to test this, is:

1. In ~/.config/companion-nodejs/ I've moved the v3.99 and v4.0 directories to old3.99 and old4.0.
   • I'm not sure v3.99 is actually meaningful here, but better safe than sorry.
   • This is on assumption that those contained something like the config info that was being improperly interpreted/migrated forward.
2. I checked out e7d7ae3 (a commit from a week-ish ago).
3. I did yarn install && yarn dist:webui && yarn dev. On running I get the "Some modules are missing version info" warning and when I click on each instance/connection I see "(Missing)" for the version info -- which seemingly is the bug as originally reported, local modules not getting 'dev' filled in.
4. Now, I repeat step 1 and move the v4.0 directory to new4.0.
   • There's no v3.99 to move at this point.
5. Then I checked out upstream/main, which is specifically 97ff1cb as the currently latest commit.
6. Then I did yarn install && yarn dist:webui && yarn dev again. On running, I still get the "Some modules are missing version information." warning, and clicking on individual connections yields "Module version: (Missing)".

That all appears to me to indicate this still doesn't quite work. But I am extremely uncertain that I've accurately replicated the initial state of my system, that the improved migration code was supposed to be able to handle!

Please let me know either what I'm doing wrong if I'm doing it wrong (very plausible), or let me know what files I can upload so you can test this yourself.

[Julusian]

This is the same as what I am seeing.
I think I want it to be this way, as it makes it an explicit user operation to fetch the modules. This puts the code we download and run a little bit more in the hands of users, rather than doing it without their knowledge.
Its only one click after all, so isn't much of a burden once users look at this page