refactor loading and error

[cbarber]

No description provided.

[bencates]

I am not a fan of adding react-recompose as a dependency here, especially given that this codebase serves as a sandbox for the interns to learn in. I don't feel like it adds much, but it does a lot to obfuscate what the HOCs are doing and how they're being applied. The more of the codebase we're able to do without extra dependencies, the smaller the number of concepts and amount of documentation a reader will need to load into their brain to understand what they're looking at.

[bencates]

Why do these need to take ErrorMessage and LoadingMessage? Those placeholder components are consistent throughout the application, so I think it'd be cleaner to bake them into the respective HOCs.

[cbarber]

That's a good point. My first grep of the usages of if (loading) / if (error) showed some alternative code paths, but those ended up being FaCC's that would be better solved by bubbling up their graphql fragments. I think defaulting the component seems reasonable.

[bencates]

I'm not a fan of using the compose function here. It's not saving us much in the way of complexity or line count, but it is an extra third-party dependency and an extra concept the reader will have to understand.

Suggested change

	export default compose(
	renderWhileError(ErrorMessage),
	renderWhileLoading(LoadingMessage),
	withFragment({ location: getBathroomCode }),
	)(Bathroom);
	const BathroomWithError = renderWhileError(Bathroom, ErrorMessage);
	const BathroomWithErrorAndLoading = renderWhileLoading(BathroomWithError, LoadingMessage);
	const BathroomWithErrorLoadingAndFragment = withFragment(BathroomWithErrorAndLoading, { location: getBathroomCode });
	export default BathroomWithErrorLoadingAndFragment

[cbarber]

I disagree. It saves us from extra churn when we want to inject/reorder HoCs e.g. if in your example we switch to loading preceding error I should rename three variables.

[bencates]

Again, I think react-compose obscures more than it reveals here

Suggested change

	export const renderWhileError = errorComponent =>
	compose(
	setPropTypes({
	error: PropTypes.bool.isRequired,
	}),
	branch(({ error }) => !!error, renderComponent(errorComponent)),
	);
	export const renderWhileError = Component, ErrorComponent => {
	const ComponentWithError = (error, ...props) => (
	error
	? <ErrorComponent />
	: <Component {...props} )/>
	);
	ComponentWithError.propTypes({
	...Component.propTypes,
	error: PropTypes.bool.isRequired,
	}
	return ComponentWithError;
	}

[bencates]

Repeating a comment from above, I think it would be cleaner to import { ErrorComponent } here instead of requiring it to be passed in.

[cbarber]

I am not a fan of adding react-recompose as a dependency here, especially given that this codebase serves as a sandbox for the interns to learn in. I don't feel like it adds much, but it does a lot to obfuscate what the HOCs are doing and how they're being applied.

I agree that there isn't much pay off here for the simple loading/error HoCs. Our pay off is likely further along when we start refactoring lifecycle/state functionality.

I'm picking recompose over roll your own here since it provides conceptual building blocks that describe what is happening. We don't have the expectation that beginners will think too hard about JSX and what it is doing under the hood, just use it as a pattern to generate DOM. I think the compose into a sequence of HoCs reads as a description of behavior that you don't need to think too deeply about to start using.

The more of the codebase we're able to do without extra dependencies, the smaller the number of concepts and amount of documentation a reader will need to load into their brain to understand what they're looking at.

I can get behind the fewer deps and less to learn. I was surprised to see we have a 6MB package when I splitting webpacker/webpack.

I'm going to mark this as DoNotMerge for now until we have some time to mull this over.

[bencates]

We don't have the expectation that beginners will think too hard about JSX and what it is doing under the hood, just use it as a pattern to generate DOM. I think the compose into a sequence of HoCs reads as a description of behavior that you don't need to think too deeply about to start using.

I couldn't disagree more. The ability to remain ignorant of what's happening under the hood is a privilege! One that can only be earned through often painful experience with the conceptual model.

Beginners, in my experience, are deeply curious about what's happening under the hood and rightly hesitant to lean on abstractions they don't really understand yet. I'd even go so far as to say that an instinct for when abstractions can be trusted is one of the main dividing lines between beginner and intermediate.

[cbarber]

The ability to remain ignorant of what's happening under the hood is a privilege! One that can only be earned through often painful experience with the conceptual model.

I agree that its a privilege, but feel its one we strive to provide for all of the engineers that will maintain our software. Its not something we gatekeep behind unraveling every technology we lean on to develop modern software. That's a journey we're all still taking.

Beginners, in my experience, are deeply curious about what's happening under the hood and rightly hesitant to lean on abstractions they don't really understand yet. I'd even go so far as to say that an instinct for when abstractions can be trusted is one of the main dividing lines between beginner and intermediate.

I don't think using abstractions and curiosity of how the abstractions work are mutually exclusive. I feel like the abstractions we depend on naturally lead to why questions. Those why questions are the rabbit hole we take to further our understanding.

We should take this meta conversation to the mentor group to further refine our abstraction demarcation point.