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 Bug That Skips doiuse When Used With An Empty Configuration #170

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LorenzoBloedow
Copy link

@LorenzoBloedow LorenzoBloedow commented Aug 11, 2023

BUG: postcss will skip doiuse if the configuration is in object format and the options for doiuse is set as an empty object.
Minimal repro: https://stackblitz.com/edit/vitejs-vite-f8xwph

A few things to note:
Plugin author (doiuse) -> exports plugin with options as the only parameter
postcss-load-config -> responsible for loading the configuration and returning a config object with a plugins field
postcss -> responsible for executing the transformation by using the result of postcss-load-config (config.plugins);


THE FOLLOWING HAPPENS IN postcss-load-config:

options = the postcss config file (e.g.: postcss.config.js)

if options.plugin is an array:
1. store the plugins as is (plugins in arrays are stored as a require call, meaning the result is an import).
2. if plugin.postcss is exactly true, store the plugin as the result of calling the default import of the plugin without any options.
3. if plugin.postcss is not true but is still a truthy value, store the plugin as plugin.postcss (I think this is for old plugin shapes).

else if options.plugins is an object:
if options.plugins[plugin] (the options for a plugin, where plugin is the name of the plugin) is an empty object:
postcss-load-config will store the plugin as a default import of the plugin. (KEEP THIS IN MIND)
else:
postcss-load-config will store the plugin as the result of calling the default import of the plugin with the options

After all this, store the results in an array.


THE FOLLOWING HAPPENS IN postcss:

  1. if the array containing the plugins is inside a one-element array, flatten it.

  2. if the property postcss exists in the top-level of the plugin and is a function, let it be the plugin itself

  3. This is a loop from postcss that loops through every plugin:

for (let i of plugins) {
	if (i.postcss === true) {
		i = i()
	} else if (i.postcss) {
        	i = i.postcss
	}

	if (typeof i === 'object' && Array.isArray(i.plugins)) {
		normalized = normalized.concat(i.plugins)
	} else if (typeof i === 'object' && i.postcssPlugin) {
		normalized.push(i)
	} else if (typeof i === 'function') {
		normalized.push(i)
	} else if (typeof i === 'object' && (i.parse || i.stringify)) {
		if (process.env.NODE_ENV !== 'production') {
			throw new Error(
				'PostCSS syntaxes cannot be used as plugins. Instead, please use ' +
				'one of the syntax/parser/stringifier options as outlined ' +
				'in your PostCSS runner documentation.'
			)
		}
	} else {
		throw new Error(i + ' is not a PostCSS plugin')
	}
}

Basically all you need to know about the code above is that doiuse currently fails the first if-else-if block and gets pushed as the unwrapped function to normalized (your local array of plugins).

This is solely because you wrapped DoIUse in an index function, and if you follow the code flow above, you'll notice that DoIUse
will arrive at the loop as a reference to the the wrapper function, since it doesn't actually contain the members of
DoIUse because it hasn't been instantiated, i.postcss will be undefined, and the reference will just be a regular function.

So the result is: probably somewhere down the chain, postcss will execute your plugin but, since it's wrapped, an instance of the plugin will be returned and nothing will actually get executed leading to your plugin getting sneakily skipped, without even a warning!


WHY THIS (PROBABLY) SHOULDN'T BE FIXED IN POSTCSS OR POSTCSS-LOAD-CONFIG
I took a better look at the source code for both of these projects and realized that postcss-load-config will not execute plugins with an empty configuration because that's the way it identifies plugins that take the shape of TransformCallback, Plugin, and Processor which are all valid plugin types that can't be executed by postcss-load-config

Why can't they be executed?
TransformCallback: This is the most simple type, being just a function without any wrappers, if executed by postcss-load-config it will just execute the plugin before postcss has a chance to.
Plugin: It's a simple object without a [[call]] slot, can't be called.
Processor: Also a simple object.

When compared to PluginCreator for example, we can see why postcss-load-config chose this strategy.
It's because PluginCreator accepts options and can be safely called by postcss-load-config to return either a Plugin or a Processor which will then be passed to postcss.

Unfortunately, by choosing to identify objects this way, we can't differentiate objects without configuration from wrapped objects without configuration, and thus, we can't unwrap it, leaving postcss with an invalid plugin type.


Other solutions:

  • Document the fact that you can't use an empty object as the configuration for this plugin.
  • When finally executing the plugin in postcss, check if it returned another plugin by creating another Processor but ignoring any errors thrown because that'd just mean the plugin isn't wrapped.
  • This is all I got for now, I'll update this if anyone has any other ideas

I will also be opening an issue on the postcss-load-config repo to know why this distinction between empty configurations is a thing and if we can change it.

TLDR: doiuse is being skipped when used with empty options while in an object configuration because postcss-load-config can't reliably unwrap doiuse and the result makes postcss confused

Possibly related to #152
Fixes this comment
Fixes this comment
Fixes this comment

@clshortfuse
Copy link
Collaborator

clshortfuse commented Sep 19, 2023

Thanks for the PR!

I'd really hard to gauge what you're trying to accomplish. It would have been best to file an issue first targeting this. Rewriting the entire structure of the files and changing their imports does not sound necessary. It sounds like you can accomplish what you want with a static property or function, or just reframe the exports which is for use with CommonJS.

Can you build a test that would fail on the current branch, but pass on the PR? It would better serve to fix the issue with more minimal changes.

@LorenzoBloedow
Copy link
Author

Hey there!
Sorry for the huge and confusing explanation, I'll admit when coming back to it after almost two months, even I got a little confused :P

I went ahead and implemented a new test that fails on the current branch but passes on the PR like you asked.

It sounds like you can accomplish what you want with a static property or function

Do you mean something like this?

export default function index(...options) {
  const doIUse = new DoIUse(...options);
  this.postcss = doIUse;
  return doIUse;
}

I tried it but it failed the test.

@clshortfuse
Copy link
Collaborator

You don't have to rewrite the whole structure. Basically, the commonjs import expects postcss to be property of the default export that calls the function.

You can just do this:

import DoIUse from '../lib/DoIUse.js';

/**
 * @param  {ConstructorParameters<typeof DoIUse>} options
 * @return {DoIUse}
 */
export default function index(...options) {
  return new DoIUse(...options);
}

+index.postcss = new DoIUse().postcss;

I should probably rewrite the postcss and info functions to be static, since that's how it's probably supposed to work. But for this one issue, it's easier to add the one-liner.

@clshortfuse clshortfuse self-assigned this Oct 4, 2023
@clshortfuse clshortfuse self-requested a review October 4, 2023 17:57
Copy link
Collaborator

@clshortfuse clshortfuse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only modify the exports/index.js file match the old 5.x export (see PR comment).

@clshortfuse clshortfuse added this to the 6.0.3 milestone Oct 4, 2023
@LorenzoBloedow
Copy link
Author

Done! Sorry for the delay

test/postcss-plugin.js Outdated Show resolved Hide resolved
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.

Add link to browserslist docs to readme
2 participants