chore: Promisify connecting and model parsing, use constants for connect views #248
Conversation
Anemy
changed the title
| }); | ||
| } else { | ||
| this._saveConnection(currentConnection); | ||
| } catch (error) { /* Ignore saving invalid connection string */ } |
There was a problem hiding this comment.
I think we should be handling errors that might occur when saving connections, however it's saving the connection in storage mixin in a pretty unhandleable way at the moment so I don' think there's much we can do right now.
|
|
||
| currentConnection.name = currentSaved.name; | ||
| currentConnection.color = currentSaved.color; | ||
| currentConnection.lastUsed = currentSaved.lastUsed; |
There was a problem hiding this comment.
I think we should look into what we really want to be doing when swapping between the connection form and connection string. I think ideally it shouldn't be doing anything here. Not going to address in this pr.
|
ah I ran my tests on the wrong branch locally. moment and I'll get them back up and running |
| } else { | ||
| this._saveRecent(currentConnection); | ||
| } | ||
| this.state.isValid = true; |
There was a problem hiding this comment.
is it intentional that we mutate the state in the same object? what about just:
this.state = { ...this.state, isValid: true, ...etc }
There was a problem hiding this comment.
also why we don't do setState here?
There was a problem hiding this comment.
There are two types of actions to change reflux state:
const isValid = this.state.isValid;
this.trigger(this.state);
this.setState({
isValid: false,
});
They drove me crazy because I didn't get why sometimes only one of those works for the plugin and tests. I hope we refactor the connection model soon and get rid of the ampersand model and then from reflux in the compass connct.
| this.state.isURIEditable = false; | ||
| this.state.customUrl = this.state.currentConnection.driverUrl; | ||
|
|
||
| currentConnection.lastUsed = new Date(); |
There was a problem hiding this comment.
this part and the following if is really hard to put in context. have no clue what is supposed to do, i think putting this in a method with a good name and comment would help a ton. What save means here, what is the difference between saveConnection and saveRecent?
There was a problem hiding this comment.
Yeah probably naming could be better :) _saveRecent calls _saveConnection later in code. _saveConnection saves connection to reflux store.
| } else { | ||
| this._saveRecent(currentConnection); | ||
| } | ||
| this.state.isValid = true; |
There was a problem hiding this comment.
There are two types of actions to change reflux state:
const isValid = this.state.isValid;
this.trigger(this.state);
this.setState({
isValid: false,
});
They drove me crazy because I didn't get why sometimes only one of those works for the plugin and tests. I hope we refactor the connection model soon and get rid of the ampersand model and then from reflux in the compass connct.
| this.state.isURIEditable = false; | ||
| this.state.customUrl = this.state.currentConnection.driverUrl; | ||
|
|
||
| currentConnection.lastUsed = new Date(); |
There was a problem hiding this comment.
Yeah probably naming could be better :) _saveRecent calls _saveConnection later in code. _saveConnection saves connection to reflux store.
|
Looks like requiring the store (from the components for the constants) messes with our mocha tests since it's trying to require electron and the mocha tests don't have that context. I think we can either move the constants into another file which they all require which is low effort, or we can move all the tests to karma which has the electron runtime for testing. I think it's probably nice if all tests run with one tester so I'm going to see if moving all to karma is easy, otherwise I'll move them into another file. |
Using async/await here should hopefully help us catch a few more errors that might occur.
Added a constant for the connection
viewType.A bit of function simplification as well, with some more early returns and breaking out functions into connection string vs connection form handlers. Tried to avoid any logic changes here.