fix(gatsby-plugin-postcss): Add required additional level of nesting for postcss-loader options.

[richardxia]

Description

This is a proposed fix for #27417, which I just reported with a more detailed description of the problem itself.

The issue appears to be this line of gatsby-plugin-postcss:

gatsby/packages/gatsby-plugin-postcss/src/gatsby-node.js

Line 33 in 3a3c3e7

options: { sourceMap: !isProduction, ...postcssOptions },

The postcssOptions object is being used with the spread (...) syntax, meaning that its properties are being included directly to the object literal passed to the options property of the PostCSS loader. This means that the options property evaluates to { sourceMap: !isProduction, plugins: [/* plugin objects */] }.

This is invalid because the postcss-loader only accepts three top-level options: execute, postcssOptions, and sourceMap. The plugins property belongs to the postcssOptions object, but because of the spread operator, it's getting hoisted up a level.

Given that the variable name in this Gatsby plugin is spelled exactly as postcssOptions, matching the postcss loader option name, I am assuming that this was a mistake and that the original intent was to pass in the entirety of the postcssOptions object into the postcss loader.

I tested this by hot-editing the version of this file in my node_modules/ directory with the exact same change to confirm that this fixes it.

Documentation

This fixes the plugin to actually match the behavior described by the documentation in https://github.com/gatsbyjs/gatsby/blob/3a3c3e7f0942578b5ee113056d4883c7c277bdd3/docs/docs/post-css.md and https://github.com/gatsbyjs/gatsby/blob/3a3c3e7f0942578b5ee113056d4883c7c277bdd3/packages/gatsby-plugin-postcss/README.md.

Related Issues

Fixes #27417.

[richardxia]

Given that the variable name in this Gatsby plugin is spelled exactly as postcssOptions, matching the postcss loader option name, I am assuming that this was a mistake and that the original intent was to pass in the entirety of the postcssOptions object into the postcss loader.

I retract this. I went digging a bit further and found out that postcss-loader made some very large, backwards-incompatible changes to its API last month with its 4.0.0 release, which involved moving its plugin option down a level into postcssOptions, which was newly created with this 4.0.0 release.

Given that it looks like Gatsby already made the leap 12 days ago to bump from postcss-loader 3.0.0 to 4.0.2, it looks like I just got unlucky and am probably the first person to attempt to use Gatsby with PostCSS 4.0 with additional plugins.

[richardxia]

Wait, this doesn't actually look right. In the 4.0.0 release of postcss-loader, sourceMap was left alone at the top level and not put inside of the new postcssOptions object. Let me see why an extra sourceMap property is getting placed inside of postcssOptions in the tests.

[richardxia]

I renamed this argument in order to not confuse it with the new postcssOptions that postcss-loader actually has now. Because the sourceMap option is still a sibling to postcssOptions in postcss-loader, we actually have to put the plugins property in another level of nesting within the actual object named postcssOptions and not what we had here previously.

As you can see below, the full hierarchy is now:

postcssLoaderOptions === {
  postcssOptions: {
    plugins: [],
  },
  sourceMap: true,
}

[LekoArts]

LGTM 👍 Thank you very much for sending this in!

[richardxia]

Thanks for the quick review!

[LekoArts]

Published in gatsby-plugin-postcss@3.0.2

[richardxia]

Actually, it looks like this didn't fully fix it. Apologies for not fully testing this again after I made significant changes to my fix. Originally, this PR was just a one-line change to unflatten the spread operator on the original line 33 (the one that constructs the options to postcss-loader), and I was able to test it by just hot-editing the installed version of this package in node_modules/. I did not, however, test this again after I made more significant changes to my PR, and it looks like this is a problem at the integration level, not the unit level.

I'm still hitting the exact same error message when I follow the steps in #27417. However, from putting some extra console.log() statements in the version of this library that's actually installed, I'm seeing that the issue is that when this onCreateWebpackConfig() function is called, it is still getting plugins passed in to the postcssLoaderOptions. This results in my postcssLoaderOptions looking like the following when it gets spread into the PostCSS loader options object:

postcssLoaderOptions === {
  postcssOptions: {
    plugins: [/* function object representing postcss-preset-env */],
  },
  plugins: [],
}

I'm not sure where that plugins property is coming from, since the unit test case doesn't have it in the mocked input data, and I don't know the Gatsby/Webpack stack enough to know how these loaders actually get called in practice. I'm now suspecting that the delete postcssLoaderOptions.postcssOptions.plugins statement, which I wrapped in an if statement in this PR, probably should not have been wrapped in an if statement. It probably should have continued to delete the plugins property in the outer object, not the inner object. However, I'd like some confirmation from someone on the Gatsby team who is more familiar with this.

I can at least prepare a followup PR that restores the original level of nesting of that delete statement, and we can continue the discussion there. Sorry again for the trouble and for the published release you made which doesn't actually fix the issue!