Skip to content
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

fix(nextjs): Remove experimental wrapping of getStaticPaths #5561

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Aug 10, 2022

Ref: #5505

This removes getStaticPaths from our WIP data-fetchers auto wrapping experiment. According to the nextjs docs, unlike getStaticProps, it never runs at runtime, so it shouldn't have been included to begin with.

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-wrapping-getStaticPaths branch from a608c33 to 2fa6855 Compare August 10, 2022 22:51
@lobsterkatie lobsterkatie requested a review from lforst August 10, 2022 23:23
@lforst lforst force-pushed the kmclb-nextjs-stop-wrapping-getStaticPaths branch from 2fa6855 to 32a35c1 Compare August 11, 2022 08:53
@lforst
Copy link
Member

lforst commented Aug 11, 2022

I rebased this onto my changes in #5546. Hope that's okay.

@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-wrapping-getStaticPaths branch from 32a35c1 to 103a026 Compare August 11, 2022 23:21
@lobsterkatie lobsterkatie force-pushed the kmclb-nextjs-stop-wrapping-getStaticPaths branch from 103a026 to e00510f Compare August 11, 2022 23:28
@lobsterkatie
Copy link
Member Author

I rebased this onto my changes in #5546. Hope that's okay.

I appreciate the effort!

It wasn't building, so I ended up re-rebasing it, because that felt easier than debugging it. In the process, I discovered two other places where your sonarlint did in my "give it a name, then return it" strategy for making functions easier to reason about. It's out of the scope of this PR to fix it, so I left it alone (plus, I know your linter is just going to yell at you again and you're going to want to fix it again and it's dumb for us to just keep switching it back and forth every time one of us touches this code). I'm bringing it up, though, just because I realized it actually perfectly illustrates exactly what I'm talking about, as trivial an example as it is.

export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn']): GSProps['wrappedFn'] {
  const wrappedGSProps = async function (context: GSProps['context']): Promise<GSProps['result']> {
    return callOriginal<GSProps>(origGetStaticProps, context);
  };

  return wrappedGSProps;
}

If I'm coming at this code fresh, I glance at that and I can immediately tell you what's getting returned (both type and meaning), plus it's very clear what lines are the inner function and which lines are the outer function.

export function withSentryGetStaticProps(origGetStaticProps: GSProps['fn']): GSProps['wrappedFn'] {
  return async function (context: GSProps['context']): Promise<GSProps['result']> {
    return callOriginal<GSProps>(origGetStaticProps, context);
  };
}

If I'm coming at this version fresh and I glance at it, I'm not going to get any of that information nearly as immediately. Even for something this tiny, here's what my brain would sound like if it were narrating its process:

Okay, what does this function do? Well, first lemme see what it returns. callOriginal, okay, I'll have to see what that does. Oh, wait, no - that's not what the function is returning. There's another function there, and IT's what's returning the results of callOriginal. Okay, so what's the outer function returning? Oh, it's returning this inner function. Okay, but what IS this inner function? Hmmm, no name, okay, what's the outer function called? Okay, so something's happening with getStaticProps. Lemme see what each of these functions takes as arguments. origGetStaticProps. Oh, and the innermost thing was callOriginal. Does it get passed to that innermost function? Yup, it does. Okay, so then what's getting passed to the function in the middle? Something which is a promise? Oh, no, that's the return type, wrong meaning of colon...

And of course, I'm exaggerating a little here, because this function is pretty easy to figure out, things are well named, it's got a good docstring, etc, etc. But if it were a more complicated function, or the naming weren't great, or if it were missing documentation... I actually would have to do all that, pay attention to everything (including a lot of stuff I don't much care about), before I ended up with more or less the same information I got almost right away when looking at the first version.

Does that make sense?

@lobsterkatie lobsterkatie merged commit fbb44d5 into master Aug 12, 2022
@lobsterkatie lobsterkatie deleted the kmclb-nextjs-stop-wrapping-getStaticPaths branch August 12, 2022 00:20
@lforst lforst mentioned this pull request Aug 13, 2022
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants