diff --git a/src/lib/cloud-manager-hoc.jsx b/src/lib/cloud-manager-hoc.jsx
index 129e7416c66eb0797d4e2212374cb0d2c1c690cd..6ca6b7500fc22cb4e27b8ee8c741d81b80e35f13 100644
--- a/src/lib/cloud-manager-hoc.jsx
+++ b/src/lib/cloud-manager-hoc.jsx
@@ -21,48 +21,48 @@ const cloudManagerHOC = function (WrappedComponent) {
this.cloudProvider = null;
}
componentDidMount () {
- if (this.canUseCloud(this.props)) {
+ if (this.shouldConnect(this.props)) {
this.connectToCloud();
}
}
componentDidUpdate (prevProps) {
- // TODO add disconnection logic when loading a new project
+ // TODO need to add cloud provider disconnection logic and cloud data clearing logic
+ // when loading a new project e.g. via file upload
// (and eventually move it out of the vm.clear function)
- // e.g. was previously showingWithId but no longer, but we need
- // to check that we don't disconnect when saving a project to the
- // server.
- // If we couldn't use cloud data before, but now we can, try opening a cloud connection
- if (this.canUseCloud(this.props) && !this.canUseCloud(prevProps)) {
+ if (this.shouldConnect(this.props) && !this.shouldConnect(prevProps)) {
this.connectToCloud();
- return;
}
- // TODO add scratcher check
-
- if ((this.props.username !== prevProps.username) ||
- (this.props.projectId !== prevProps.projectId)) {
- this.connectToCloud();
+ if (this.shouldDisconnect(this.props, prevProps)) {
+ this.disconnectFromCloud();
}
}
componentWillUnmount () {
- if (this.cloudProvider) {
- this.cloudProvider.requestCloseConnection();
- this.cloudProvider = null;
- }
+ this.disconnectFromCloud();
}
canUseCloud (props) {
- return props.cloudHost && props.username && props.isShowingWithId &&
- props.vm && props.username && props.projectId;
+ // TODO add a canUseCloud to pass down to this HOC (e.g. from www) and also check that here.
+ // This should cover info about the website specifically, like scrather status
+ return !!(props.cloudHost && props.username && props.vm && props.projectId);
}
- connectToCloud () {
- if (this.cloudProvider && this.cloudProvider.connection) {
- // Already connected
- return;
- }
-
+ shouldConnect (props) {
+ return !this.isConnected() && this.canUseCloud(props) && props.isShowingWithId;
+ }
+ shouldDisconnect (props, prevProps) {
+ return this.isConnected() &&
+ ( // Can no longer use cloud or cloud provider info is now stale
+ !this.canUseCloud(this.props) ||
+ (props.projectId !== prevProps.projectId) ||
+ (props.username !== prevProps.username)
+ );
// TODO need to add provisions for viewing someone
// else's project in editor mode
+ }
+ isConnected () {
+ return this.cloudProvider && !!this.cloudProvider.connection;
+ }
+ connectToCloud () {
this.cloudProvider = new CloudProvider(
this.props.cloudHost,
this.props.vm,
@@ -70,6 +70,13 @@ const cloudManagerHOC = function (WrappedComponent) {
this.props.projectId);
this.props.vm.setCloudProvider(this.cloudProvider);
}
+ disconnectFromCloud () {
+ if (this.cloudProvider) {
+ this.cloudProvider.requestCloseConnection();
+ this.cloudProvider = null;
+ this.props.vm.setCloudProvider(null);
+ }
+ }
render () {
const {
/* eslint-disable no-unused-vars */
diff --git a/src/lib/cloud-provider.js b/src/lib/cloud-provider.js
index 2e159054c2d01d5d04600db3d3daebba18b07619..f268ae473f7bde4425b2678530a6e07ebf317dd2 100644
--- a/src/lib/cloud-provider.js
+++ b/src/lib/cloud-provider.js
@@ -54,8 +54,10 @@ class CloudProvider {
log.info(`Received websocket message: ${messageString}`);
// Multiple commands can be received, newline separated
messageString.split('\n').forEach(message => {
- const parsedData = this.parseMessage(JSON.parse(message));
- this.vm.postIOData('cloud', parsedData);
+ if (message) { // .split can also contain '' in the array it returns
+ const parsedData = this.parseMessage(JSON.parse(message));
+ this.vm.postIOData('cloud', parsedData);
+ }
});
}
diff --git a/test/unit/util/cloud-manager-hoc.test.jsx b/test/unit/util/cloud-manager-hoc.test.jsx
index 854f9b596b1d73d0e790f98f54132502b4fcad5b..7414fc7cb6dd3de0420cab86c7660b421448f4fe 100644
--- a/test/unit/util/cloud-manager-hoc.test.jsx
+++ b/test/unit/util/cloud-manager-hoc.test.jsx
@@ -6,7 +6,13 @@ import {mount} from 'enzyme';
import VM from 'scratch-vm';
import {LoadingState} from '../../../src/reducers/project-state';
import CloudProvider from '../../../src/lib/cloud-provider';
-jest.mock('../../../src/lib/cloud-provider');
+const mockCloudProviderInstance = {
+ connection: true,
+ requestCloseConnection: jest.fn()
+};
+jest.mock('../../../src/lib/cloud-provider', () =>
+ jest.fn().mockImplementation(() => mockCloudProviderInstance)
+);
import cloudManagerHOC from '../../../src/lib/cloud-manager-hoc.jsx';
@@ -36,6 +42,7 @@ describe('CloudManagerHOC', () => {
vm = new VM();
vm.setCloudProvider = jest.fn();
CloudProvider.mockClear();
+ mockCloudProviderInstance.requestCloseConnection.mockClear();
});
test('when it mounts, the cloud provider is set on the vm', () => {
const Component = () => (<div />);
@@ -50,6 +57,7 @@ describe('CloudManagerHOC', () => {
);
expect(vm.setCloudProvider.mock.calls.length).toBe(1);
expect(CloudProvider).toHaveBeenCalledTimes(1);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
});
test('when cloudHost is missing, the cloud provider is not set on the vm', () => {
@@ -115,5 +123,105 @@ describe('CloudManagerHOC', () => {
});
expect(vm.setCloudProvider.mock.calls.length).toBe(1);
expect(CloudProvider).toHaveBeenCalledTimes(1);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
+ });
+
+ test('projectId change should not trigger cloudProvider connection unless isShowingWithId becomes true', () => {
+ const Component = () => <div />;
+ const WrappedComponent = cloudManagerHOC(Component);
+ const mounted = mount(
+ <WrappedComponent
+ cloudHost="nonEmpty"
+ store={stillLoadingStore}
+ username="user"
+ vm={vm}
+ />
+ );
+ mounted.setProps({
+ projectId: 'a different id'
+ });
+ expect(vm.setCloudProvider.mock.calls.length).toBe(0);
+ expect(CloudProvider).not.toHaveBeenCalled();
+ mounted.setProps({
+ isShowingWithId: true,
+ loadingState: LoadingState.SHOWING_WITH_ID
+ });
+ expect(vm.setCloudProvider.mock.calls.length).toBe(1);
+ expect(CloudProvider).toHaveBeenCalledTimes(1);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
+ });
+
+ test('when it unmounts, the cloud provider is set on the vm', () => {
+ const Component = () => (<div />);
+ const WrappedComponent = cloudManagerHOC(Component);
+ const mounted = mount(
+ <WrappedComponent
+ cloudHost="nonEmpty"
+ store={store}
+ username="user"
+ vm={vm}
+ />
+ );
+
+ expect(CloudProvider).toHaveBeenCalled();
+ const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
+
+ mounted.unmount();
+
+ // vm.setCloudProvider is called twice,
+ // once during mount and once during unmount
+ expect(vm.setCloudProvider.mock.calls.length).toBe(2);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
+ expect(requestCloseConnection).toHaveBeenCalledTimes(1);
+ });
+
+ test('projectId changing should trigger cloudProvider disconnection', () => {
+ const Component = () => <div />;
+ const WrappedComponent = cloudManagerHOC(Component);
+ const mounted = mount(
+ <WrappedComponent
+ cloudHost="nonEmpty"
+ store={store}
+ username="user"
+ vm={vm}
+ />
+ );
+
+ expect(CloudProvider).toHaveBeenCalled();
+ const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
+
+ mounted.setProps({
+ projectId: 'a different id'
+ });
+
+ expect(vm.setCloudProvider.mock.calls.length).toBe(2);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
+ expect(requestCloseConnection).toHaveBeenCalledTimes(1);
+
+ });
+
+ test('username changing should trigger cloudProvider disconnection', () => {
+ const Component = () => <div />;
+ const WrappedComponent = cloudManagerHOC(Component);
+ const mounted = mount(
+ <WrappedComponent
+ cloudHost="nonEmpty"
+ store={store}
+ username="user"
+ vm={vm}
+ />
+ );
+
+ expect(CloudProvider).toHaveBeenCalled();
+ const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
+
+ mounted.setProps({
+ username: 'a different user'
+ });
+
+ expect(vm.setCloudProvider.mock.calls.length).toBe(2);
+ expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
+ expect(requestCloseConnection).toHaveBeenCalledTimes(1);
+
});
});
diff --git a/test/unit/util/cloud-provider.test.js b/test/unit/util/cloud-provider.test.js
index ee720044d9ce6c398a6081aa1f5de02aea534d63..3510339c01eddeb20f3cb04246ace23fd75c688f 100644
--- a/test/unit/util/cloud-provider.test.js
+++ b/test/unit/util/cloud-provider.test.js
@@ -88,6 +88,16 @@ describe('CloudProvider', () => {
expect(vmIOData[0].varUpdate.value).toEqual('value');
});
+ test('onMessage with newline at the end', () => {
+ const msg1 = JSON.stringify({
+ method: 'set',
+ name: 'name1',
+ value: 'value'
+ });
+ cloudProvider.onMessage({data: `${msg1}\n`});
+ expect(vmIOData[0].varUpdate.name).toEqual('name1');
+ });
+
test('onMessage with multiple commands', () => {
const msg1 = JSON.stringify({
method: 'set',