Refactored ProfileFormContainer to remove boilerplate

[alicewriteswrongs]

What are the relevant tickets?

this closes #712

What's this PR do?

Just abstracts out a little bit of boilerplate. We had a lot of helper methods on ProfileFormFields like the following:

dispatchSomeAction: Function = (arg: someType): void => {
  const { dispatch } = this.props;
  dispatch(someAction(arg));
};

this abstracts that pattern, so instead we do:

createActionHelper: Function = (actionCreator: Function): (...args: any) => void  => {
  const { dispatch } = this.props;
  return (...args) => dispatch(actionCreator(...args));
};
createActionHelper(someAction);

We can do this for most of the action creator helpers we had on ProfileFormFields, although anything with more specialized logic than the example above (e.g. saveProfile) needs to be left alone.

Where should the reviewer start?

Read over the changes.

How should this be manually tested?

Make sure there are no regressions on any of the profile components (on /profile and /users).

Any background context you want to provide?

Screenshots (if appropriate)

What GIF best describes this PR or how it makes you feel?

[alicewriteswrongs]

I left this as-is because we use this.setProfileStep on ProfilePage, but now that I think about it we could just use createActionHelper there too...

[alicewriteswrongs]

another question is if it makes sense to put this in another file, so that we could re-use a similar pattern on other container components (same for the method below). then we'd just do:

import { createActionHelper } from '../util/redux';

createActionHelper.call(this, someAction);

[alicewriteswrongs]

or createActionHelper could take dispatch as a parameter, hmmm

[alicewriteswrongs]

I think I'm going to do that so we can write unit tests

[gsidebo]

just wondering: why can't we just import dispatch where it's needed? why does it need to be passed down as a prop?

[alicewriteswrongs]

Because dispatch is defined at runtime by the store, so we wouldn't be able to import something.

I think the only alternative to passing it down as a prop would be sticking it on the global object (or on window or something) which would be way yucky.

[gsidebo]

this has been irking me for a while now. thanks for doing something about it! i'll try to take a look later today

[alicewriteswrongs]

haha, me too, every time I had to add another one of those boilerplate functions it felt dirty and wrong.

[alicewriteswrongs]

@gsidebo I think this is ready for review whenever you're free

[gsidebo]

(continuing conversation from outdated diff re: dispatch)

sticking dispatch on window or something would indeed be yucky, but i consider the current approach (pass as a prop to All The Things) to be pretty yucky as well. i stumbled across this thread while looking into this: reduxjs/redux#916. any reason we aren't just using connect() for the components that need to use dispatch? we do it in static/js/containers/App.js, and the community seems to recommend this approach

[alicewriteswrongs]

Right now we're following basically the container component approach, where we have container components (like App.js and ProfilePage.js and so forth) which are connected to the store, and they pass data from the store and callbacks using the dispatch function to presentational components, which are never connected to the store and pretty much just receive props and render UI with them. I think there's some value to retaining the separation of concerns there, where the container components have all the logic around creating helper methods for dispatching actions, doing higher level data fiddling (figuring out which profile to pass to a presentational component, for instance), and so on, and the presentational components just worry about being given, say, a profile and some callbacks and drawing the right UI with those inputs.

Does that make sense? We do have quite a number of different components which are connected to the store (basically everything in static/js/containers) right now, I'm not exactly sure what would be different about what you're suggesting.

[gsidebo]

perhaps a little controversial, but one way to avoid this key-value repetition would be to use import * as ui_actions from 'path/to/actions'. might make the most sense to put that in the util file rather than here. that way you could just pass a list of strings, eg: ["setWorkDialogVisibility", "setWorkDialogIndex", "clearProfileEdit"] and we wouldn't have this slightly awkward list-of-lists-with-repeated-names, nor the large import statement above. yes, that would open up the possibility of passing a function name that doesn't exist in actions/ui, but we could make the list of action names a property of this class and test its validity in a unit test

[gsidebo]

another (cleaner?) option would be to pass in a list of the action functions an use .name. the util function could just use actionCreator.name to get the string

EDIT: yeah, this one is cleaner

[alicewriteswrongs]

when I was initially writing this I ran into some issue with func.name, can't figure out what it was now though! haha

I'll rewrite it to use that.

[gsidebo]

if that doesn't work for whatever reason, i think you can also use func.prototype.constructor.name or something gross like that

[gsidebo]

makes sense. i was confusing myself a bit when i wrote that comment. my main issue was that we're passing this mound of functions down the entire component tree as props, many of whom would never need those functions. i think i understand the reasoning now, though

side comment: i'm not sure why we're doing this export default connect(ProfileFormContainer.mapStateToProps)(ProfilePage) on every class that inherits from ProfileFormContainer instead of just doing that on the actual ProfileFormContainer class. no need to change that for this PR, though

[alicewriteswrongs]

I just tried that, and it does not work. It seems that subclassing a class connected with to the store with connect does not make the child class connected to the store, so unfortunately we need that on each class subclassing ProfileFormFields.

We could write a mix-in to add that I think, but those are always a bit messy.

[gsidebo]

LGTM pending CI build 👍

[alicewriteswrongs]

haha hold your horses, haven't written tests yet :P

[gsidebo]

[alicewriteswrongs]

@gsidebo Ok tests are done!

[gsidebo]

wait... these are the same function (createSimpleActionHelpers and createAsyncActionHelpers). i wondered this before but forgot about it: why not just return the promise from dispatch in all cases? if the client doesn't do anything with the return value, that's on them. don't think it has any performance penalty. then we could combine these two into one called createActionHelpers.

also, i think we should rename actionManifest to actions or actionList

[alicewriteswrongs]

We could turn all the action creators into async ones, but we'd have to refactor all of the synchronous ones to make that work (which seems out of scope for this pr). I think we also rely on a lot of those actions being dispatched synchronously, so I imagine that would be a really big refactor.

agree re: actionManifest, yeah, it's not really a manifest any more, haha.

[gsidebo]

i'm missing something here. the body of createSimpleActionHelpers and createAsyncActionHelpers is literally the same thing. i added some confusion with my comment about returning the dispatch value. in brief: either we're missing something that should be differentiating these two functions or we should combine them, right?

[alicewriteswrongs]

they're differentiated because they have different return types, createSimpleActionHelpers returns this:

Array<{[k: string]: (...args: any) => void}>

an array of objects with a string key and a function that takes some arguments and returns undefined. these are the simple synchronous action creators that just take a value and dispatch a change to the store.

createAsyncActionHelpers has the following return type:

Array<{[k: string]: (...args: any) => Promise}>

so an array of objects mapping a string to a function that takes some arguments and returns a Promise. these are the async action creators.

note: we're using the redux-thunk middleware for asynchronous actions.

[gsidebo]

so specifying the return type actually has an affect on the functionality? i thought it was just a sort of 'wrapper' to validate types going in and out. i'm so confused - it looks like we have 2 blocks of the same exact javascript, but we're getting something different from each block because we've added flow typing. i'm willing to thumbs-up this, but i'd like to understand this first

[alicewriteswrongs]

the flow type annotations are all stripped out before the code is parsed by Babel for transpilation, so this is a case where we have to declare another function to get the typechecker to pass (i.e. there is no runtime typechecking except for what JS does on it's own). Alternatives would require checking the return type in a lot of places to see if it is a Promise before calling .then on it, I think this is a better solution since the actual functionality is in the shared createActionHelper function anyway and these two functions are basically just wrappers around that.

[gsidebo]

can you leave a brief comment for possibly-confused devs (read: me) looking at this in the future? aside from that, this looks good

EDIT: added thought - can you just define the return actionManifest.map(actionCreator => ( ... statement as a separate function with no type checking, or will that trip up Babel? you could just name it createActionHelpers. if that sounds doable, cool. if not, cool

[alicewriteswrongs]

we'll get "can't call method .then on possibly undefined value" errors, I had that setup initially but couldn't find an easy way to get it to pass.

[gsidebo]

👍