React router #343
|
|
@ -50,11 +50,13 @@ class ShowAsset extends React.Component {
|
||||||
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
// update the showAsset data in the store
|
// update the showAsset data in the store
|
||||||
const { assets } = this.props;
|
const { assets } = this.props;
|
||||||
const assetId = `a#${name}#${claimId}`;
|
const assetId = `a#${name}#${claimId}`;
|
||||||
if (assets[assetId]) { // case: the asset data already exists
|
const existingAssetRecord = assets[assetId];
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
let { error: assetError, name, claimId, shortId, claimData } = assets[assetId];
|
if (existingAssetRecord) { // case: the asset data already exists
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
|
let { error: assetError, name, claimId, shortId, claimData } = existingAssetRecord;
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
this.props.onShowExistingAsset(assetId, assetError, name, claimId, shortId, claimData);
|
this.props.onShowExistingAsset(assetId, assetError, name, claimId, shortId, claimData);
|
||||||
} else { // case: the asset data does not exist yet
|
} else { // case: the asset data does not exist yet
|
||||||
this.props.onShowNewAsset(assetId, name, claimId);
|
console.log('error: there should be an existing record');
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
|
// this.props.onShowNewAsset(assetId, name, claimId);
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
componentWillUnmount () {
|
componentWillUnmount () {
|
||||||
|
|
|
||||||
|
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
I think you are still creating more work than necessary with this. In my opinion I also think I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request.
I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
Ok, I think I'm getting closer. I was able to do away with Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
|
|||||||
|
|
@ -38,7 +38,7 @@ class ShowChannel extends React.Component {
|
||||||
|
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
|
|||||||
console.log('new request');
|
console.log('new request');
|
||||||
this.props.onNewChannelRequest(requestId, requestName, requestChannelId);
|
this.props.onNewChannelRequest(requestId, requestName, requestChannelId);
|
||||||
}
|
}
|
||||||
onRepeatChannelRequest ({ id, error, name, claimId }) {
|
onRepeatChannelRequest ({ id, error, data: { channelName, longChannelClaimId} }) {
|
||||||
|
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
|
|||||||
// if error, return and update state with error
|
// if error, return and update state with error
|
||||||
if (error) {
|
if (error) {
|
||||||
return this.props.onRequestError(error);
|
return this.props.onRequestError(error);
|
||||||
|
|
|
||||||
|
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
Same comments above about Same comments above about `previousRequest`. I think a more understandable approach would just be:
```
if (!channel) this.props.onNewChannelRequest(...)
```
See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done. See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
|
|||||||
|
|
@ -1,6 +1,6 @@
|
||||||
import { call, put, takeLatest } from 'redux-saga/effects';
|
import { call, put, takeLatest } from 'redux-saga/effects';
|
||||||
import * as actions from 'constants/show_action_types';
|
import * as actions from 'constants/show_action_types';
|
||||||
import { addAssetRequest, updateShowAsset, addChannelRequest, updateShowChannel, updateFileAvailability, updateDisplayAssetError } from 'actions/show';
|
import { addAssetRequest, updateShowAsset, showNewAsset, addChannelRequest, updateShowChannel, updateFileAvailability, updateDisplayAssetError } from 'actions/show';
|
||||||
import { UNAVAILABLE, AVAILABLE } from 'constants/asset_display_states';
|
import { UNAVAILABLE, AVAILABLE } from 'constants/asset_display_states';
|
||||||
import { checkFileAvailability, triggerClaimGet } from 'api/fileApi';
|
import { checkFileAvailability, triggerClaimGet } from 'api/fileApi';
|
||||||
import { getLongClaimId, getShortId, getClaimData } from 'api/assetApi';
|
import { getLongClaimId, getShortId, getClaimData } from 'api/assetApi';
|
||||||
|
|
@ -15,26 +15,12 @@ function* newAssetRequest (action) {
|
||||||
yield put(addAssetRequest(id, error.message, name, null));
|
yield put(addAssetRequest(id, error.message, name, null));
|
||||||
}
|
}
|
||||||
if (success) {
|
if (success) {
|
||||||
return yield put(addAssetRequest(id, null, name, longId));
|
yield put(addAssetRequest(id, null, name, longId));
|
||||||
|
return yield put(showNewAsset(id, name, longId));
|
||||||
}
|
}
|
||||||
yield put(addAssetRequest(id, message, name, null));
|
yield put(addAssetRequest(id, message, name, null));
|
||||||
};
|
};
|
||||||
|
|
||||||
function* newChannelRequest (action) {
|
|
||||||
const { id, name, channelId } = action.data;
|
|
||||||
let success, message, data;
|
|
||||||
try {
|
|
||||||
({success, message, data} = yield call(getChannelData, name, channelId));
|
|
||||||
} catch (error) {
|
|
||||||
yield put(addChannelRequest(id, error.message, null));
|
|
||||||
}
|
|
||||||
if (success) {
|
|
||||||
console.log('api/channel/data/ response:', data);
|
|
||||||
return yield put(addChannelRequest(id, null, data));
|
|
||||||
}
|
|
||||||
yield put(addChannelRequest(id, message, null));
|
|
||||||
}
|
|
||||||
|
|
||||||
function* getAssetDataAndShowAsset (action) {
|
function* getAssetDataAndShowAsset (action) {
|
||||||
const {id, name, claimId} = action.data;
|
const {id, name, claimId} = action.data;
|
||||||
// if no error, get short Id
|
// if no error, get short Id
|
||||||
|
|
@ -94,6 +80,21 @@ function* retriveFile (action) {
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
function* newChannelRequest (action) {
|
||||||
|
const { id, name, channelId } = action.data;
|
||||||
|
let success, message, data;
|
||||||
|
try {
|
||||||
|
({success, message, data} = yield call(getChannelData, name, channelId));
|
||||||
|
} catch (error) {
|
||||||
|
yield put(addChannelRequest(id, error.message, null));
|
||||||
|
}
|
||||||
|
if (success) {
|
||||||
|
console.log('api/channel/data/ response:', data);
|
||||||
|
return yield put(addChannelRequest(id, null, data));
|
||||||
|
}
|
||||||
|
yield put(addChannelRequest(id, message, null));
|
||||||
|
}
|
||||||
|
|
||||||
export function* watchNewAssetRequest () {
|
export function* watchNewAssetRequest () {
|
||||||
yield takeLatest(actions.ASSET_REQUEST_NEW, newAssetRequest);
|
yield takeLatest(actions.ASSET_REQUEST_NEW, newAssetRequest);
|
||||||
};
|
};
|
||||||
|
|
|
||||||
I think you are still creating more work than necessary with this. In my opinion
previousRequestshouldn't even exist. In themapStateToPropsyou should be able to map theassetfrom your state into the component. If!assetthen make the request.I also think
onShowNewAssetandonNewRequestcan be combined. More specifically I don't thinkonShowNewAssetis needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".