feat(core): allow configureWebpack to return undefined

[yorkie]

Motivation

() => {
  return {
    name: 'configure-server-side-rendering',
    configureWebpack(_, isServer) {
      if (isServer) {
        return {
          plugins: [
            new webpack.ProvidePlugin({
              'URLSearchParams': ['url', 'URLSearchParams'],
            }),
          ],
        }
      } else {
        return {};
      }
    },
  };
}

The above is a custom plugin to add a ProvidePlugin for server-side only, in this case, we don't wanna change the config for the client, but I have to return an empty object, so this PR is to improve the writing plugins in this case.

Have you read the Contributing Guidelines on pull requests?

Yes, and I will follow the commit guideline if this change LGTY :)

Test Plan

I'm not sure how to test this :)

Related PRs

None.

[netlify]

✅ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 85d47f6

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/623945a3226bbc0008a558ab

😎 Browse the preview: https://deploy-preview-6784--docusaurus-2.netlify.app

[github-actions]

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	60
🟢 Accessibility	100
🟢 Best practices	92
🟢 SEO	100
🟢 PWA	90

Lighthouse ran on https://deploy-preview-6784--docusaurus-2.netlify.app/

[Josh-Cena]

I don't really like this change. It is good practice in JS to ensure symmetric return, and ESLint has a rule (consistent-return) to enforce this. Relying on the implicit undefined return may be the source of errors and is generally a code smell. Docusaurus wants to promote good coding practices.

Still I would leave the final decision to @slorber

[slorber]

[ERROR] TypeError: Cannot destructure property 'mergeStrategy' of 'configureWebpack(...)' as it is undefined.

In any case, we want a better error message than the existing one:

Relying on the implicit undefined return may be the source of errors and is generally a code smell.

I think allowing the code above is quite convenient. Users can also explicitly return undefined.

Eventually we can force TS users to be more explicit:

  configureWebpack?: (
    config: Configuration,
    isServer: boolean,
    utils: ConfigureWebpackUtils,
    content: Content,
-  ) => Configuration & {mergeStrategy?: ConfigureWebpackFnMergeStrategy};
+  ) => (Configuration & {mergeStrategy?: ConfigureWebpackFnMergeStrategy}) | null;

JS users may still use implicit undefined and we can fail-safe in this case, or at least throw a better error message asking to return "null" explicitly?

---

@yorkie I'm willing to merge this change but I'd like to have a test + a correct TS type

Let us know if you want help

[Josh-Cena]

No, the point is that this is succinct:

configureWebpack(isServer) {
  if (isServer) {
    return {
      foo: 'bar';
    };
  }
}

But this is verbose:

configureWebpack(isServer) {
  if (isServer) {
    return {
      foo: 'bar';
    };
  }
  return {};
}

It doesn't matter if you return {}, null, or undefined. As long as you explicitly return something, the syntax tax is the same. However, encouraging users to not return anything is encouraging bad coding practice that we don't want.

I think we could improve the error message, but nothing more. Returning {} is both semantically and practically good enough.

[slorber]

I see

IMHO this is more config than real user code
I don't think the ESLint rules should apply in the same way for config so I'd still like to provide this shortcut to make user life easier. Users are responsible for their own codebase coding practices, we shouldn't annoy them to ask for an empty object where this could work without.

Not 100% the same but there are multiple places where we accept "partial" configs and try to infer a good general behavior

[yorkie]

@yorkie I'm willing to merge this change but I'd like to have a test + a correct TS type
Let us know if you want help

I will add tests and fix typings later, thank you @slorber :)

[Josh-Cena]

I still think that config is user code, but let's put stylistic preferences aside and talk about potential mistakes. What if the user made a mistake and forgot to write the return statement? These plugins are where the validation system can't cover. What about giving a nicer error message asking the user to explicitly return {}?

[slorber]

What if the user made a mistake and forgot to write the return statement?

I don't know, this is something I would do myself on purpose.

If I am against the idea of using implicit return on purpose, I can bring my own eslint config or use TS to prevent it from happening.

We shouldn't force a user to absolutely have to return {} if they prefer using implicit undefined returns, like it's the case for 2/3 users of this PR :)

If things can work on the 1st try without asking users to read another error message and do additional tasks, that doesn't seem like a bad idea to me.

[Josh-Cena]

We shouldn't force a user to absolutely have to return {} if they prefer using implicit undefined returns, like it's the case for 2/3 users of this PR :)

Fair, I would be fine with Configuration | undefined then. Just totally unsure of the value🤷‍♂️ Null-safety is not something I like in general (I'd rather let it crash on undefined), unless a nullish value actually has semantic significance (as a "hole", for example).

[slorber]

Slightly related choice: React allowing to render undefined: reactwg/react-18#75

[Josh-Cena]

After some pondering, I decided to not make typing changes. Otherwise, the return type would be Configuration | undefined | void which clearly stands out as an anti-pattern. We would warrant null-safety, but people who opt into stronger types should also write their code in a more sensible fashion.

@slorber If you read up that reactwg/react-18#75 thread, undefined is allowed for pragmatic reasons, because otherwise types are hard to write. That doesn't happen for us. "This should be the job of linters" doesn't warrant why React should allow undefined; it warrants why React can allow undefined, because it would be caught by other tools anyways. Implicitly returning undefined should still be frowned upon by all tooling, let it be TypeScript, ESLint, or React itself.

[Josh-Cena]

Merging this. I changed my mind because I realized we have null-safe lifecycles elsewhere, so at least we have precedents.