New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial source management UI #1406

Merged
merged 1 commit into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@chnn
Contributor

chnn commented Nov 14, 2018

  • Adds a simple UI for listing sources, adding a new source, deleting a source, and switching the active source
  • Normalizes the schema used to store sources in Redux
  • Makes Redux the state of truth for the currently active source, rather than the query param (the query param will stay synced via the SetActiveSource component)
  • Fixes the inability to navigate backwards after switching to certain pages

screen shot 2018-11-14 at 2 37 07 pm

screen shot 2018-11-14 at 2 37 04 pm

@chnn chnn force-pushed the sources-page branch from 919c296 to 8e9736e Nov 14, 2018

@@ -120,7 +119,6 @@ class LogsPage extends Component<Props, State> {
public async componentDidMount() {
try {
await this.props.getSources()

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

This was removed since it already happens when the app first boots up (every route in the app is a child of the GetSources component, which fetches sources before rendering its children).

@chnn chnn force-pushed the sources-page branch from 8e9736e to 9083f3e Nov 14, 2018

@jaredscheib

a number of minor considerations that don't merit responding if you feel otherwise. straightforward, clear, and clean work! nice!

const {query} = location
const defaultSource = sources.find(s => s.default)
const id = query.sourceID || _.get(defaultSource, 'id', 0)
const mapStateToProps = (state: AppState) => {

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor
  1. mstp?
  2. explicit return type
@@ -0,0 +1,82 @@
import React, {PureComponent} from 'react'

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

y'all still organizing these under comment categories?

constructor(props: Props) {
super(props)
this.resolveActiveSource()

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

could side effects from within the constructor be problematic?

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

I actually moved it from the componentDidMount hook... the issue was that the children of this component (the entire rest of the app) was rendering before an active source had been resolved (since the component has already rendered once by the time the componentDidMount hook has been resolved). Putting the active source resolution in the constructor instead fixed a few errors in the app where components were expecting a source to exist.

I can't think of any reason why it would create problems to be in the constructor, but it does seem a bit strange.

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

Is it setting state in other components? I don't have full context/understanding but just recall things like this historically: https://reactjs.org/docs/react-component.htmlAvoid introducing any side-effects or subscriptions in the constructor. For those use cases, use componentDidMount() instead.

getState: GetState
) => {
const sourcesLink = getState().links.sources
const sources = await readSourcesAJAX(sourcesLink)

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

anything catching this and all the following?

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

for the most part they end up getting caught (or should be caught) by the components that dispatch the action. My thinking was that different components might want to handle errors from the same action differently

}
export const deleteSource = (sourceID: string) => async (
dispatch: any,

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

any desired?

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

whoops, nice catch

}
}
const mdtp = dispatch => ({

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

possibly type?

case 'REMOVE_SOURCE': {
const sources = {...state.sources}
delete sources[action.payload.sourceID]

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

a more functional style would be .filter, but this works just as well (and should be quicker). i approve!

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

I opted for the janky-immutability since there's no built-in filter method on object types (it would have to be the lodash filter or something).

was it you that was sharing a link to immer a while ago? super cool idea for reducers

case 'SET_SOURCES': {
const sources = {...state.sources}
for (const source of action.payload.sources) {

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

there's prob a lodash function for this.. escaping me atm

return null
}
const source = state.sources.sources[activeSourceID]

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

any chance this lookup fails at some level of nesting?

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

it may return undefined, but it should never fail with a property of undefined error

}
export const getSources = (state: AppState): Source[] =>
Object.values(state.sources.sources)

This comment has been minimized.

@jaredscheib

jaredscheib Nov 14, 2018

Contributor

you loooove Object.values :)

This comment has been minimized.

@chnn

chnn Nov 14, 2018

Contributor

haha totally a consequence of indexing collections by id in Redux

}
}
const mstp = (state: AppState) => ({

This comment has been minimized.

@AlirieGray

AlirieGray Nov 14, 2018

Contributor

could type the return as ConnectStateProps

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

I think any type error should be caught by the generics that are passed to connect

}
/*
Given an object of query parameter keys and values, updates any corresponding

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

nice

export const setActiveSource = (
activeSourceID: string | null
): SetActiveSourceAction => ({
type: 'SET_ACTIVE_SOURCE',

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

could use an enum for the action types instead of hard-coded strings for these actions.

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

I've been using strings instead of enums since it cuts down on the amount of boilerplate for creating actions while still offering comparable type safety in a reducer

await deleteSourceAJAX(source)
dispatch(removeSource(sourceID))

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

maybe check that the delete source call was successful, and only dispatch removeSource if it was successful?

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

good call 👍

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

actually I think the function would throw an error at that point, so the dispatch line would never be hit

export const getSourceHealth = async (url: string) => {
export const readSources = async (url): Promise<Source[]> => {

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

why readSources instead of getSources?

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

for consistency with the CRUD vocabulary, and since get tends to have so many different meanings

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

got it, thanks for clarifying 👍

} catch (error) {
this.setState({creationStatus: RemoteDataState.Error})
onNotify(sourceCreationFailed(error.toString()))
}

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

should the creationStatus be set to Done after the try catch block?

This comment has been minimized.

@chnn

chnn Nov 15, 2018

Contributor

since onHide gets called, the entire component is unmounted. I think setting the creationStatus would cause one of the those “setState on an unmounted component” errors.

}
}
private handleInputChange = (e: ChangeEvent<HTMLInputElement>) => {

This comment has been minimized.

@AlirieGray

AlirieGray Nov 15, 2018

Contributor

nice

@AlirieGray

AlirieGray approved these changes Nov 15, 2018 edited

looks great!

@chnn

This comment has been minimized.

Contributor

chnn commented Nov 15, 2018

awesome, thank you both

@chnn chnn force-pushed the sources-page branch from 9083f3e to 5efbc0d Nov 15, 2018

@chnn chnn merged commit 81c0e53 into master Nov 15, 2018

3 checks passed

ci/circleci: gotest Your tests passed on CircleCI!
Details
ci/circleci: jstest Your tests passed on CircleCI!
Details
continuous-integration/jenkins/branch This commit looks good
Details

@chnn chnn deleted the sources-page branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment