search

bitfocus / companion-module-figure53-qlab-advance

MIT License
3 stars 7 forks source link

Qlab merge prep #36

Closed krocheck closed 3 years ago

krocheck commented 4 years ago

@istnv I validated that all actions in qlab are present in qlab-advance, so the only thing to merge seems to be the upgrade script and ultimate rename of this module to qlab. Let me know if this looks good then I'll proceed with all the core shuffling (in a branch) to test this out.

krocheck commented 4 years ago

So one issue I ran into with testing the upgrades is that applyConfig is run prior to the upgrades, therefore the default of true for useTCP is set per that function. We either need to change that default in applyConfig or move applyConfig to the init function.

istnv commented 4 years ago

I am currently re-arranging the home office: the MacBook is on the shelf at the moment and my dev system is in the middle of a system/os upgrade. My comments here are from inspecting the code since I can't pull this branch yet.

The useTCP==undefined section in applyConfig can be removed. It was a very early fix before having a proper addUpgradeScript. qlabfb started as a TCP only module, with UDP mode added later.

Change self.useTCP = config.useTCP; at the top of function instance( to self.useTCP = false. Set it to false in applyConfig if it doesn't exist.

Move applyConfig call to after the upgrade scripts in function instance( to catch any changes made there.

krocheck commented 4 years ago

Moving applyConfig to after the addUpgradeScripts call in function instance( won't change the execution. That function only registers the upgrade scripts. The scripts themselves fire between function instance( and instance.prototype.init(. I'm not seeing any reasons moving applyConfig to init( would adversely affect execution. Thoughts?

I'll cleanup the default within applyConfig.

istnv commented 4 years ago

I had an issue at one point where certain instance variables weren't always initialized before access. A lot of code moved during the file split, though, and applyConfig was one of the results. Moving to init should be fine.