feat: Add cancel connection attempt COMPASS-4393 #257
Conversation
| @@ -7,6 +7,7 @@ const Actions = Reflux.createActions({ | |||
| validateConnectionString: { sync: true }, | |||
| onAuthSourceChanged: { sync: true }, | |||
| onAuthStrategyChanged: { sync: true }, | |||
| onCancelConnectionAttemptClicked: { sync: true }, | |||
There was a problem hiding this comment.
Unrelated from PR .. In other plugins we have a mixture of sync true and false, i don't get what that's suppose to do though, what happens when is sync? does it accumulate state changes and only emit them at the end?
There was a problem hiding this comment.
I think something in the store of compass-connect which is potentially a bug inducing practice is a combination of directly updating items on the state and later triggering a store update event store.trigger( with store.setState. I think we should only use one method, maybe setState to ensure we are performing the correct updates at the right time.
As for sync: true and sync: false it looks like it's a reflux pattern with triggering the store: https://github.com/reflux/refluxjs/tree/master/docs/actions#async-vs-sync-actions it's a bit confusing to me to be honest. How I understand it, all of these sync actions automatically cause a store trigger (and maybe more since some of these methods call trigger manually).
| (item) => item._id === connection._id | ||
| ); | ||
|
|
||
| toDestrioy.destroy({ | ||
| toDestroy.destroy({ | ||
| success: () => { |
There was a problem hiding this comment.
this is Ampersand right :/?
There was a problem hiding this comment.
Looks like it yup https://ampersandjs.com/docs/#ampersand-model-destroy
There was a problem hiding this comment.
Connection storage is a big mix of ampersand and storage mixin - maybe we rewrite a storage module once we monorepo?
There was a problem hiding this comment.
This is amazing! Finally i don't have to wait 30 seconds each time i connect to a cluster that is down :).
Before merging i'd consider an alternative to the uuid and tracking of multiple attempts. I think is better to clarify the UX too, I'm not super convinced that allowing the connection to be interrupted non-intentionally is a good thing.
|
@mcasimir updated how we handle connecting and added a modal view for connecting. Still tweaking the animation, so maybe we can look at everything except for that part now? |
|
@mcasimir updated the animation: connecting.mp4good to merge? |
src/stores/index.js
Outdated
| // Compass relies on `compass-connect` showing a progress | ||
| // bar, which is hidden after the instance information is loaded | ||
| // in another plugin. | ||
| this.StatusActions.showIndeterminateProgressBar(); |
There was a problem hiding this comment.
shouldn't this go in the other plugin? like oneOfTheRegistry.on('connected', () => showTheBar)
There was a problem hiding this comment.
maybe we would be able to avoid it after we load all the details?
There was a problem hiding this comment.
Yes - or it shouldn't be shown at all.
I'd like to defer that to this ticket if that's cool though: https://jira.mongodb.org/browse/COMPASS-4608
src/stores/index.js
Outdated
| const runConnect = promisify(dataService.connect.bind(dataService)); | ||
| const connectedDataService = await runConnect(); | ||
| const dataService = new DataService(connection); | ||
| this.connectingConnectionAttempt = createConnectionAttempt(dataService); |
There was a problem hiding this comment.
Why don't we encapsulate the whole connection in the connection attempt?
this.connectingConnectionAttempt = createConnectionAttempt(connection);
this.dataService = await this.connectingConnectionAttempt.connect();
and move all the new Dataservice() stuff etc down in the ConnectionAttempt class?
There was a problem hiding this comment.
I started to do this, but I found it makes it a bit harder to test just passing the connectionModel to the ConnectionAttempt class. We'd have to be stubbing out a lot of functionality or add some dep injection. I think maybe we leave it like this for now since it's not much of a code change and makes testing a bit easier without having to change up the ConnectionAttempt implementation.
src/stores/index.js
Outdated
|
|
||
| if (!connectedDataService || !this.state.isConnecting) { | ||
| return; | ||
| } | ||
|
|
||
| const currentConnection = this.state.currentConnection; |
There was a problem hiding this comment.
this part and the part below currentConnection.lastUsed ... (which seems to be related), id really move it in a separate method: saveEstabilishedConnection or something similar, is not really clear why this thing is here, i'm just guessing that's the function of this code.
There was a problem hiding this comment.
Added an _onConnectSuccess method. Could break it down a bit more, but I think it's a start.
src/stores/index.js
Outdated
|
|
||
| if (!connectedDataService || !this.state.isConnecting) { | ||
| return; | ||
| } | ||
|
|
||
| const currentConnection = this.state.currentConnection; |
There was a problem hiding this comment.
why do we re-read the connection from the state instead of using connection from above here, shouldn't that be the source of truth?
There was a problem hiding this comment.
Chatted about this on video - it's because the connection model passed to this method isn't always the same as the one on the state - the connect with connection string for instance mutates a new connection derived from the state which isn't the same. Something we'll address in future and clean up since it's probably causing bugs or could cause bugs.
| }, | ||
|
|
||
| /** | ||
| * Resets URL validation. | ||
| */ | ||
| onConnectionFormChanged() { | ||
| const currentConnection = this.state.currentConnection; |
There was a problem hiding this comment.
sorry for being so picky .. i'm aware that all of this was here already, feel free to ignore this if is too silly, but what about renaming currentConnection, prop.currentConnection and state.currentConnection etc .. into something that is more close to reality. currentConnection makes me think about the actual connection object from the driver (like the mongoclient) or similar .., would not really think of that being just an object with the connection attributes. What about directly .connectionModel instead?
There was a problem hiding this comment.
Good suggestion - it confused me at first too. Going to rename currentConnection in a different pr since it's everywhere. I'll open it after this is merged if that's cool.
…t success into function
COMPASS-4393
This PR adds the ability to cancel a connection attempt while it is still being established. Previously we would show an overlay which disabled all of the ui.
We also add leafygreen-ui buttons for showing the connect form buttons now. And we add mongodb-runner to help with some connectivity testing.
Cancelling a connection attempt:
connecting.experience.mp4
Added a slight reveal to the background of the modal after record that video to make it a little smoother revealing