React router #343

Merged
bones7242 merged 96 commits from react-router into master 2018-02-15 08:02:17 +01:00
10 changed files with 93 additions and 135 deletions
Showing only changes of commit aedc636241 - Show all commits

View file

@ -1,37 +1,30 @@
const logger = require('winston'); const logger = require('winston');
module.exports = { module.exports = {
returnErrorMessageAndStatus: function (error) { handleErrorResponse: function (originalUrl, ip, error, res) {
neb-b commented 2018-02-09 22:30:08 +01:00 (Migrated from github.com)
Review

Why changing to 200?

Why changing to 200?
bones7242 commented 2018-02-13 05:13:54 +01:00 (Migrated from github.com)
Review

I am trying to create a pattern where the server responds with a 200 as long as it processed the request correctly, and then responds with a success and a message or data field. I.e. {"success": "false", "message": "connection refused. The daemon may not be running"}. This way, if the client wanted some data but the daemon was off, it would get a 200 response that the request was completed, and then the client could check the response to see if it was successful or not and whether the data is present. My goal was to help myself (and api users) to distinguish between an error where something was unavailable but the request was handled properly (resulting in a 200) vs a request that I thought would be handled but wasn't, resulting in some other error. Thoughts?

I am trying to create a pattern where the server responds with a 200 as long as it processed the request correctly, and then responds with a `success` and a `message` or `data` field. I.e. `{"success": "false", "message": "connection refused. The daemon may not be running"}`. This way, if the client wanted some data but the daemon was off, it would get a `200` response that the request was completed, and then the client could check the response to see if it was successful or not and whether the data is present. My goal was to help myself (and api users) to distinguish between an error where something was unavailable but the request was handled properly (resulting in a `200`) vs a request that I thought would be handled but wasn't, resulting in some other error. Thoughts?
neb-b commented 2018-02-13 05:45:11 +01:00 (Migrated from github.com)
Review

I think proper error codes would still be preferable. Even if the request is handled properly, there is still an error. If I see a 200 response, I assume my request was successful. The fetch api handles errors really nicely too, so I think if anything you are creating more work.

fetch('spee.ch/xxx')
  .then((data) => ...)         // successful
  .catch((error) => ...)       // not successful



fetch('spee.ch/xxx')
  .then((data) => {           // maybe successful?
    if (!data.success) {
      throw new Error()
   }
    ...                       // ok now I know it was successful
  })
  .catch((err) => ... )       // not successful

In my opinion it doesn't really matter if a request was handled successfully. If the data doesn't make it, it still fails. You can still add info with the error message and code.

I think proper error codes would still be preferable. Even if the request is handled properly, there is still an error. If I see a 200 response, I assume my request was successful. The `fetch` api handles errors really nicely too, so I think if anything you are creating more work. ``` fetch('spee.ch/xxx') .then((data) => ...) // successful .catch((error) => ...) // not successful fetch('spee.ch/xxx') .then((data) => { // maybe successful? if (!data.success) { throw new Error() } ... // ok now I know it was successful }) .catch((err) => ... ) // not successful ``` In my opinion it doesn't really matter if a request was handled successfully. If the data doesn't make it, it still fails. You can still add info with the error message and code.
bones7242 commented 2018-02-13 21:21:24 +01:00 (Migrated from github.com)
Review

That makes a lot of sense, and I was running into a lot of the latter situation where I had to check success in addition to checking for errors. I updated it and that cleaned up the response handling a lot.

That makes a lot of sense, and I was running into a lot of the latter situation where I had to check `success` in addition to checking for errors. I updated it and that cleaned up the response handling a lot.
let status, message; logger.error(`Error on ${originalUrl}`, module.exports.useObjectPropertiesIfNoKeys(error));
// check for daemon being turned off
if (error.code === 'ECONNREFUSED') {
status = 200;
message = 'Connection refused. The daemon may not be running.';
// check for thrown errors
} else if (error.message) {
status = 200;
message = error.message;
// fallback for everything else
} else {
status = 400;
message = error;
}
return [status, message];
},
handleRequestError: function (originalUrl, ip, error, res) {
logger.error(`Request Error on ${originalUrl}`, module.exports.useObjectPropertiesIfNoKeys(error));
const [status, message] = module.exports.returnErrorMessageAndStatus(error);
res
.status(status)
.render('requestError', module.exports.createErrorResponsePayload(status, message));
},
handleApiError: function (originalUrl, ip, error, res) {
logger.error(`Api Error on ${originalUrl}`, module.exports.useObjectPropertiesIfNoKeys(error));
const [status, message] = module.exports.returnErrorMessageAndStatus(error); const [status, message] = module.exports.returnErrorMessageAndStatus(error);
res res
.status(status) .status(status)
.json(module.exports.createErrorResponsePayload(status, message)); .json(module.exports.createErrorResponsePayload(status, message));
}, },
returnErrorMessageAndStatus: function (error) {
let status, message;
// check for daemon being turned off
if (error.code === 'ECONNREFUSED') {
status = 503;
message = 'Connection refused. The daemon may not be running.';
// fallback for everything else
} else {
status = 400;
if (error.message) {
message = error.message;
} else {
message = error;
};
};
return [status, message];
},
useObjectPropertiesIfNoKeys: function (err) { useObjectPropertiesIfNoKeys: function (err) {
if (Object.keys(err).length === 0) { if (Object.keys(err).length === 0) {
let newErrorObject = {}; let newErrorObject = {};

View file

@ -21,29 +21,21 @@ class NavBar extends React.Component {
checkForLoggedInUser () { checkForLoggedInUser () {
const params = {credentials: 'include'}; const params = {credentials: 'include'};
request('/user', params) request('/user', params)
.then(({success, message, data}) => { .then(({ data }) => {
if (success) {
this.props.onChannelLogin(data.channelName, data.shortChannelId, data.channelClaimId); this.props.onChannelLogin(data.channelName, data.shortChannelId, data.channelClaimId);
} else {
console.log(message);
}
}) })
.catch(error => { .catch(error => {
console.log('request encountered an error', error); console.log('/user error:', error.message);
}); });
} }
logoutUser () { logoutUser () {
const params = {credentials: 'include'}; const params = {credentials: 'include'};
request('/logout', params) request('/logout', params)
.then(({success, message}) => { .then(() => {
if (success) {
this.props.onChannelLogout(); this.props.onChannelLogout();
} else {
console.log(message);
}
}) })
.catch(error => { .catch(error => {
console.log('request encountered an error', error); console.log('/logout error', error.message);
}); });
} }
handleSelection (event) { handleSelection (event) {

View file

@ -8,28 +8,22 @@ function* retrieveFile (action) {
const name = action.data.name; const name = action.data.name;
const claimId = action.data.claimId; const claimId = action.data.claimId;
// see if the file is available // see if the file is available
let success, message, isAvailable; let isAvailable;
try { try {
({ success, message, data: isAvailable } = yield call(checkFileAvailability, name, claimId)); ({ data: isAvailable } = yield call(checkFileAvailability, name, claimId));
} catch (error) { } catch (error) {
return yield put(updateDisplayAssetError(error.message)); return yield put(updateDisplayAssetError(error.message));
}; };
if (!success) {
return yield put(updateDisplayAssetError(message));
}
if (isAvailable) { if (isAvailable) {
return yield put(updateFileAvailability(AVAILABLE)); return yield put(updateFileAvailability(AVAILABLE));
} }
yield put(updateFileAvailability(UNAVAILABLE)); yield put(updateFileAvailability(UNAVAILABLE));
// initiate get request for the file // initiate get request for the file
try { try {
({ success, message } = yield call(triggerClaimGet, name, claimId)); yield call(triggerClaimGet, name, claimId);
} catch (error) { } catch (error) {
return yield put(updateDisplayAssetError(error.message)); return yield put(updateDisplayAssetError(error.message));
}; };
if (!success) {
return yield put(updateDisplayAssetError(message));
}
yield put(updateFileAvailability(AVAILABLE)); yield put(updateFileAvailability(AVAILABLE));
}; };

View file

@ -6,32 +6,28 @@ import { getChannelData } from 'api/channelApi';
function* newAssetRequest (action) { function* newAssetRequest (action) {
const { id, name, modifier } = action.data; const { id, name, modifier } = action.data;
let success, message, longId; console.log('getting asset long id');
let longId;
try { try {
({success, message, data: longId} = yield call(getLongClaimId, name, modifier)); ({data: longId} = yield call(getLongClaimId, name, modifier));
} catch (error) { } catch (error) {
console.log('error:', error);
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
return yield put(updateRequestError(message));
}
yield put(addAssetRequest(id, null, name, longId)); yield put(addAssetRequest(id, null, name, longId));
yield put(showNewAsset(name, longId)); yield put(showNewAsset(name, longId));
}; };
function* newChannelRequest (action) { function* newChannelRequest (action) {
const { id, name, channelId } = action.data; const { id, name, channelId } = action.data;
let success, message, data; console.log('getting channel long id');
let data;
try { try {
({success, message, data} = yield call(getChannelData, name, channelId)); ({data} = yield call(getChannelData, name, channelId));
} catch (error) { } catch (error) {
// return yield put(addChannelRequest(id, error.message, null, null, null)); // return yield put(addChannelRequest(id, error.message, null, null, null));
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
// return yield put(addChannelRequest(id, message, null, null, null));
return yield put(updateRequestError(message));
}
const { longChannelClaimId: longId, shortChannelClaimId: shortId } = data; const { longChannelClaimId: longId, shortChannelClaimId: shortId } = data;
yield put(addChannelRequest(id, null, name, longId, shortId)); yield put(addChannelRequest(id, null, name, longId, shortId));
yield put(showNewChannel(name, shortId, longId)); yield put(showNewChannel(name, shortId, longId));

View file

@ -7,27 +7,20 @@ function* getAssetDataAndShowAsset (action) {
const {id, name, claimId} = action.data; const {id, name, claimId} = action.data;
// get short Id // get short Id
console.log('getting short id'); console.log('getting short id');
let success, message, shortId; let shortId;
try { try {
({success, message, data: shortId} = yield call(getShortId, name, claimId)); ({data: shortId} = yield call(getShortId, name, claimId));
} catch (error) { } catch (error) {
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
return yield put(updateRequestError(message));
}
// if no error, get claim data // if no error, get claim data
console.log('getting claim data'); console.log('getting claim data');
success = null;
let claimData; let claimData;
try { try {
({success, message, data: claimData} = yield call(getClaimData, name, claimId)); ({data: claimData} = yield call(getClaimData, name, claimId));
} catch (error) { } catch (error) {
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
return yield put(updateRequestError(message));
}
yield put(addAssetToAssetList(id, null, name, claimId, shortId, claimData)); yield put(addAssetToAssetList(id, null, name, claimId, shortId, claimData));
yield put(updateRequestError(null)); yield put(updateRequestError(null));
} }

View file

@ -5,15 +5,12 @@ import { getChannelClaims } from 'api/channelApi';
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
function* getChannelClaimsAndShowChannel (action) { function* getChannelClaimsAndShowChannel (action) {
const { id, name, shortId, longId } = action.data; const { id, name, shortId, longId } = action.data;
let success, message, claimsData; let claimsData;
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
try { try {
({ success, message, data: claimsData } = yield call(getChannelClaims, name, longId, 1)); ({ data: claimsData } = yield call(getChannelClaims, name, longId, 1));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
} catch (error) { } catch (error) {
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
return yield put(updateRequestError(message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
yield put(addNewChannelToChannelList(id, name, shortId, longId, claimsData)); yield put(addNewChannelToChannelList(id, name, shortId, longId, claimsData));
yield put(updateRequestError(null)); yield put(updateRequestError(null));
} }
@ -24,15 +21,12 @@ export function* watchShowNewChannel () {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
function* getNewClaimsAndUpdateClaimsList (action) { function* getNewClaimsAndUpdateClaimsList (action) {
const { channelKey, name, longId, page } = action.data; const { channelKey, name, longId, page } = action.data;
let success, message, claimsData; let claimsData;
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
try { try {
({ success, message, data: claimsData } = yield call(getChannelClaims, name, longId, page)); ({ data: claimsData } = yield call(getChannelClaims, name, longId, page));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
} catch (error) { } catch (error) {
return yield put(updateRequestError(error.message)); return yield put(updateRequestError(error.message));
} }
if (!success) {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
return yield put(updateRequestError(message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
yield put(updateChannelClaims(channelKey, claimsData)); yield put(updateChannelClaims(channelKey, claimsData));
} }

neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue

View file

@ -13,18 +13,18 @@ function parseJSON (response) {
} }
/** /**
* Checks if a network request came back fine, and throws an error if not * Parses the status returned by a network request
* *
* @param {object} response A response from a network request * @param {object} response A response from a network request
* @param {object} response The parsed JSON from the network request
* *
* @return {object|undefined} Returns either the response, or throws an error * @return {object | undefined} Returns object with status and statusText, or undefined
*/ */
function checkStatus (response) { function checkStatus (response, jsonResponse) {
if (response.status >= 200 && response.status < 300) { if (response.status >= 200 && response.status < 300) {
return response; return jsonResponse;
} }
const error = new Error(jsonResponse.message);
const error = new Error(response.statusText);
error.response = response; error.response = response;
throw error; throw error;
} }
@ -37,8 +37,13 @@ function checkStatus (response) {
* *
* @return {object} The response data * @return {object} The response data
*/ */
export default function request (url, options) { export default function request (url, options) {
return fetch(url, options) return fetch(url, options)
.then(checkStatus) .then(response => {
.then(parseJSON); return Promise.all([response, parseJSON(response)]);
})
.then(([response, jsonResponse]) => {
return checkStatus(response, jsonResponse);
});
} }

View file

@ -16,7 +16,7 @@ const NO_CLAIM = 'NO_CLAIM';
module.exports = (app) => { module.exports = (app) => {
// route to check whether site has published to a channel // route to check whether site has published to a channel
app.get('/api/channel/availability/:name', ({ params }, res) => { app.get('/api/channel/availability/:name', ({ ip, originalUrl, params }, res) => {
checkChannelAvailability(params.name) checkChannelAvailability(params.name)
.then(result => { .then(result => {
if (result === true) { if (result === true) {
@ -26,35 +26,32 @@ module.exports = (app) => {
} }
}) })
.catch(error => { .catch(error => {
res.status(500).json(error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to get a short channel id from long channel Id // route to get a short channel id from long channel Id
app.get('/api/channel/short-id/:longId/:name', ({ ip, originalUrl, params }, res) => { app.get('/api/channel/short-id/:longId/:name', ({ ip, originalUrl, params }, res) => {
db.Certificate.getShortChannelIdFromLongChannelId(params.longId, params.name) db.Certificate.getShortChannelIdFromLongChannelId(params.longId, params.name)
.then(shortId => { .then(shortId => {
logger.debug('sending back short channel id', shortId);
res.status(200).json(shortId); res.status(200).json(shortId);
}) })
.catch(error => { .catch(error => {
logger.error('api error getting short channel id', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
errorHandlers.handleApiError(originalUrl, ip, error, res);
}); });
}); });
app.get('/api/channel/data/:channelName/:channelClaimId', ({ ip, originalUrl, body, params }, res) => { app.get('/api/channel/data/:channelName/:channelClaimId', ({ ip, originalUrl, body, params }, res) => {
const channelName = params.channelName; const channelName = params.channelName;
let channelClaimId = params.channelClaimId; let channelClaimId = params.channelClaimId;
if (channelClaimId === 'none') channelClaimId = null; if (channelClaimId === 'none') channelClaimId = null;
getChannelData(channelName, channelClaimId, 0) // getChannelViewData(channelName, channelId, 0) getChannelData(channelName, channelClaimId, 0)
.then(data => { .then(data => {
if (data === NO_CHANNEL) { if (data === NO_CHANNEL) {
return res.status(200).json({success: false, message: 'No matching channel was found'}); return res.status(404).json({success: false, message: 'No matching channel was found'});
} }
res.status(200).json({success: true, data}); res.status(200).json({success: true, data});
}) })
.catch(error => { .catch(error => {
logger.error('api error getting channel contents', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
errorHandlers.handleApiError(originalUrl, ip, error, res);
}); });
}); });
app.get('/api/channel/claims/:channelName/:channelClaimId/:page', ({ ip, originalUrl, body, params }, res) => { app.get('/api/channel/claims/:channelName/:channelClaimId/:page', ({ ip, originalUrl, body, params }, res) => {
@ -62,16 +59,15 @@ module.exports = (app) => {
let channelClaimId = params.channelClaimId; let channelClaimId = params.channelClaimId;
if (channelClaimId === 'none') channelClaimId = null; if (channelClaimId === 'none') channelClaimId = null;
const page = params.page; const page = params.page;
getChannelClaims(channelName, channelClaimId, page)// getChannelViewData(channelName, channelClaimId, page) getChannelClaims(channelName, channelClaimId, page)
.then(data => { .then(data => {
if (data === NO_CHANNEL) { if (data === NO_CHANNEL) {
return res.status(200).json({success: false, message: 'No matching channel was found'}); return res.status(404).json({success: false, message: 'No matching channel was found'});
} }
res.status(200).json({success: true, data}); res.status(200).json({success: true, data});
}) })
.catch(error => { .catch(error => {
logger.error('api error getting channel contents', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
errorHandlers.handleApiError(originalUrl, ip, error, res);
}); });
}); });
// route to run a claim_list request on the daemon // route to run a claim_list request on the daemon
@ -81,7 +77,7 @@ module.exports = (app) => {
res.status(200).json(claimsList); res.status(200).json(claimsList);
}) })
.catch(error => { .catch(error => {
errorHandlers.handleApiError(originalUrl, ip, error, res); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to get an asset // route to get an asset
@ -107,11 +103,11 @@ module.exports = (app) => {
res.status(200).json({ success: true, message, completed }); res.status(200).json({ success: true, message, completed });
}) })
.catch(error => { .catch(error => {
errorHandlers.handleApiError(originalUrl, ip, error, res); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to check whether this site published to a claim // route to check whether this site published to a claim
app.get('/api/claim/availability/:name', ({ params }, res) => { app.get('/api/claim/availability/:name', ({ ip, originalUrl, params }, res) => {
checkClaimNameAvailability(params.name) checkClaimNameAvailability(params.name)
.then(result => { .then(result => {
if (result === true) { if (result === true) {
@ -121,7 +117,7 @@ module.exports = (app) => {
} }
}) })
.catch(error => { .catch(error => {
res.status(500).json(error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to run a resolve request on the daemon // route to run a resolve request on the daemon
@ -131,7 +127,7 @@ module.exports = (app) => {
res.status(200).json(resolvedUri); res.status(200).json(resolvedUri);
}) })
.catch(error => { .catch(error => {
errorHandlers.handleApiError(originalUrl, ip, error, res); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to run a publish request on the daemon // route to run a publish request on the daemon
@ -151,7 +147,6 @@ module.exports = (app) => {
({fileName, filePath, fileType} = parsePublishApiRequestFiles(files)); ({fileName, filePath, fileType} = parsePublishApiRequestFiles(files));
({channelName, channelPassword} = parsePublishApiChannel(body, user)); ({channelName, channelPassword} = parsePublishApiChannel(body, user));
} catch (error) { } catch (error) {
logger.debug('publish request rejected, insufficient request parameters', error);
return res.status(400).json({success: false, message: error.message}); return res.status(400).json({success: false, message: error.message});
} }
// check channel authorization // check channel authorization
@ -193,18 +188,17 @@ module.exports = (app) => {
sendGoogleAnalyticsTiming(timingActionType, headers, ip, originalUrl, publishStartTime, publishEndTime); sendGoogleAnalyticsTiming(timingActionType, headers, ip, originalUrl, publishStartTime, publishEndTime);
}) })
.catch(error => { .catch(error => {
errorHandlers.handleApiError(originalUrl, ip, error, res); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
// route to get a short claim id from long claim Id // route to get a short claim id from long claim Id
app.get('/api/claim/short-id/:longId/:name', ({ params }, res) => { app.get('/api/claim/short-id/:longId/:name', ({ ip, originalUrl, body, params }, res) => {
db.Claim.getShortClaimIdFromLongClaimId(params.longId, params.name) db.Claim.getShortClaimIdFromLongClaimId(params.longId, params.name)
.then(shortId => { .then(shortId => {
res.status(200).json({success: true, data: shortId}); res.status(200).json({success: true, data: shortId});
}) })
.catch(error => { .catch(error => {
logger.error('api error getting short channel id', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
res.status(200).json({success: false, message: error.message});
}); });
}); });
app.post('/api/claim/long-id', ({ ip, originalUrl, body, params }, res) => { app.post('/api/claim/long-id', ({ ip, originalUrl, body, params }, res) => {
@ -216,16 +210,15 @@ module.exports = (app) => {
getClaimId(channelName, channelClaimId, claimName, claimId) getClaimId(channelName, channelClaimId, claimName, claimId)
.then(result => { .then(result => {
if (result === NO_CHANNEL) { if (result === NO_CHANNEL) {
return res.status(200).json({success: false, message: 'No matching channel could be found'}); return res.status(404).json({success: false, message: 'No matching channel could be found'});
} }
if (result === NO_CLAIM) { if (result === NO_CLAIM) {
return res.status(200).json({success: false, message: 'No matching claim id could be found'}); return res.status(404).json({success: false, message: 'No matching claim id could be found'});
} }
res.status(200).json({success: true, data: result}); res.status(200).json({success: true, data: result});
}) })
.catch(error => { .catch(error => {
logger.error('api error getting long claim id', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
errorHandlers.handleApiError(originalUrl, ip, error, res);
}); });
}); });
app.get('/api/claim/data/:claimName/:claimId', ({ ip, originalUrl, body, params }, res) => { app.get('/api/claim/data/:claimName/:claimId', ({ ip, originalUrl, body, params }, res) => {
@ -235,29 +228,27 @@ module.exports = (app) => {
db.Claim.resolveClaim(claimName, claimId) db.Claim.resolveClaim(claimName, claimId)
.then(claimInfo => { .then(claimInfo => {
if (!claimInfo) { if (!claimInfo) {
return res.status(200).json({success: false, message: 'No claim could be found'}); return res.status(404).json({success: false, message: 'No claim could be found'});
} }
res.status(200).json({success: true, data: claimInfo}); res.status(200).json({success: true, data: claimInfo});
}) })
.catch(error => { .catch(error => {
logger.error('api error getting long claim id', error); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
errorHandlers.handleApiError(originalUrl, ip, error, res);
}); });
}); });
// route to see if asset is available locally // route to see if asset is available locally
app.get('/api/file/availability/:name/:claimId', ({ ip, originalUrl, params }, res) => { app.get('/api/file/availability/:name/:claimId', ({ ip, originalUrl, params }, res) => {
const name = params.name; const name = params.name;
const claimId = params.claimId; const claimId = params.claimId;
let isAvailable = false;
db.File.findOne({where: {name, claimId}}) db.File.findOne({where: {name, claimId}})
.then(result => { .then(result => {
if (result) { if (result) {
isAvailable = true; return res.status(200).json({success: true, data: true});
} }
res.status(200).json({success: true, data: isAvailable}); res.status(200).json({success: true, data: false});
}) })
.catch(error => { .catch(error => {
errorHandlers.handleApiError(originalUrl, ip, error, res); errorHandlers.handleErrorResponse(originalUrl, ip, error, res);
}); });
}); });
}; };

View file

@ -20,7 +20,7 @@ module.exports = (app) => {
return next(err); return next(err);
} }
if (!user) { if (!user) {
return res.status(200).json({ return res.status(400).json({
success: false, success: false,
message: info.message, message: info.message,
}); });
@ -49,7 +49,7 @@ module.exports = (app) => {
if (req.user) { if (req.user) {
res.status(200).json({success: true, data: req.user}); res.status(200).json({success: true, data: req.user});
} else { } else {
res.status(200).json({success: false, message: 'user is not logged in'}); res.status(401).json({success: false, message: 'user is not logged in'});
} }
}); });
}; };

View file

@ -1,7 +1,7 @@
const logger = require('winston'); const logger = require('winston');
const { getClaimId, getLocalFileRecord } = require('../controllers/serveController.js'); const { getClaimId, getLocalFileRecord } = require('../controllers/serveController.js');
const serveHelpers = require('../helpers/serveHelpers.js'); const serveHelpers = require('../helpers/serveHelpers.js');
const { handleRequestError } = require('../helpers/errorHandlers.js'); const { handleErrorResponse } = require('../helpers/errorHandlers.js');
const lbryUri = require('../helpers/lbryUri.js'); const lbryUri = require('../helpers/lbryUri.js');
const SERVE = 'SERVE'; const SERVE = 'SERVE';
@ -92,7 +92,7 @@ module.exports = (app) => {
try { try {
({ hasFileExtension } = lbryUri.parseModifier(params.claim)); ({ hasFileExtension } = lbryUri.parseModifier(params.claim));
} catch (error) { } catch (error) {
return res.status(200).json({success: false, message: error.message}); return res.status(400).json({success: false, message: error.message});
} }
let responseType = determineResponseType(hasFileExtension, headers); let responseType = determineResponseType(hasFileExtension, headers);
if (responseType !== SERVE) { if (responseType !== SERVE) {
@ -103,14 +103,14 @@ module.exports = (app) => {
try { try {
({ claimName } = lbryUri.parseClaim(params.claim)); ({ claimName } = lbryUri.parseClaim(params.claim));
} catch (error) { } catch (error) {
return res.status(200).json({success: false, message: error.message}); return res.status(400).json({success: false, message: error.message});
} }
// parse the identifier // parse the identifier
let isChannel, channelName, channelClaimId, claimId; let isChannel, channelName, channelClaimId, claimId;
try { try {
({ isChannel, channelName, channelClaimId, claimId } = lbryUri.parseIdentifier(params.identifier)); ({ isChannel, channelName, channelClaimId, claimId } = lbryUri.parseIdentifier(params.identifier));
} catch (error) { } catch (error) {
return handleRequestError(originalUrl, ip, error, res); return res.status(400).json({success: false, message: error.message});
} }
if (!isChannel) { if (!isChannel) {
[claimId, claimName] = flipClaimNameAndIdForBackwardsCompatibility(claimId, claimName); [claimId, claimName] = flipClaimNameAndIdForBackwardsCompatibility(claimId, claimName);
@ -121,15 +121,15 @@ module.exports = (app) => {
getClaimId(channelName, channelClaimId, claimName, claimId) getClaimId(channelName, channelClaimId, claimName, claimId)
.then(fullClaimId => { .then(fullClaimId => {
if (fullClaimId === NO_CLAIM) { if (fullClaimId === NO_CLAIM) {
return res.status(200).json({success: false, message: 'no claim id could be found'}); return res.status(404).json({success: false, message: 'no claim id could be found'});
} else if (fullClaimId === NO_CHANNEL) { } else if (fullClaimId === NO_CHANNEL) {
return res.status(200).json({success: false, message: 'no channel id could be found'}); return res.status(404).json({success: false, message: 'no channel id could be found'});
} }
serveAssetToClient(fullClaimId, claimName, res); serveAssetToClient(fullClaimId, claimName, res);
// postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'success'); // postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'success');
}) })
.catch(error => { .catch(error => {
handleRequestError(originalUrl, ip, error, res); handleErrorResponse(originalUrl, ip, error, res);
// postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'fail'); // postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'fail');
}); });
}); });
@ -140,7 +140,7 @@ module.exports = (app) => {
try { try {
({ hasFileExtension } = lbryUri.parseModifier(params.claim)); ({ hasFileExtension } = lbryUri.parseModifier(params.claim));
} catch (error) { } catch (error) {
return res.status(200).json({success: false, message: error.message}); return res.status(400).json({success: false, message: error.message});
} }
let responseType = determineResponseType(hasFileExtension, headers); let responseType = determineResponseType(hasFileExtension, headers);
if (responseType !== SERVE) { if (responseType !== SERVE) {
@ -151,7 +151,7 @@ module.exports = (app) => {
try { try {
({claimName} = lbryUri.parseClaim(params.claim)); ({claimName} = lbryUri.parseClaim(params.claim));
} catch (error) { } catch (error) {
return res.status(200).json({success: false, message: error.message}); return res.status(400).json({success: false, message: error.message});
} }
// log the request data for debugging // log the request data for debugging
logRequestData(responseType, claimName, null, null); logRequestData(responseType, claimName, null, null);
@ -159,13 +159,13 @@ module.exports = (app) => {
getClaimId(null, null, claimName, null) getClaimId(null, null, claimName, null)
.then(fullClaimId => { .then(fullClaimId => {
if (fullClaimId === NO_CLAIM) { if (fullClaimId === NO_CLAIM) {
return res.status(200).render('index'); return res.status(404).json({success: false, message: 'no claim id could be found'});
} }
serveAssetToClient(fullClaimId, claimName, res); serveAssetToClient(fullClaimId, claimName, res);
// postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'success'); // postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'success');
}) })
.catch(error => { .catch(error => {
handleRequestError(originalUrl, ip, error, res); handleErrorResponse(originalUrl, ip, error, res);
// postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'fail'); // postToStats(responseType, originalUrl, ip, claimName, fullClaimId, 'fail');
}); });
}); });