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

NoWorkResult and LazyResult have very subtle differences and these can be accidentally encountered #1869

Closed
romainmenke opened this issue Aug 14, 2023 · 15 comments · Fixed by #1908

Comments

@romainmenke
Copy link
Contributor

romainmenke commented Aug 14, 2023

postcss/lib/processor.js

Lines 44 to 55 in a0a82d3

process(css, opts = {}) {
if (
this.plugins.length === 0 &&
typeof opts.parser === 'undefined' &&
typeof opts.stringifier === 'undefined' &&
typeof opts.syntax === 'undefined'
) {
return new NoWorkResult(this, css, opts)
} else {
return new LazyResult(this, css, opts)
}
}

For a full repro see : https://github.com/romainmenke/postcss-no-work-result-sourcemaps


By setting parser: null as a processing option you can force a LazyResult.

Setting null instead of undefined is a common mistake, especially when not using TypeScript.


Is it intentional that NoWorkResult and LazyResult have different CSS outputs?

@ai
Copy link
Member

ai commented Aug 14, 2023

By setting parser: null as a processing option you can force a LazyResult.

We can add extra check, send PR.

@romainmenke
Copy link
Contributor Author

I don't really want to change anything unless I fully understand this.


Is it intentional that NoWorkResult and LazyResult have different CSS outputs?

I think this is the more important question :)
I can't answer this.

@ai
Copy link
Member

ai commented Aug 14, 2023

Is it intentional that NoWorkResult and LazyResult have different CSS outputs?

It should not. We are using NoWorkResult when LazyResult will get the input === output.

@ahmdammarr
Copy link
Contributor

does this PR makes it better ?
@ai @romainmenke

@romainmenke
Copy link
Contributor Author

Thanks @ahmdammarr,

This does not fix the underlying issue that NoWorkResult and LazyResult give different results right?

@ai
Copy link
Member

ai commented Dec 29, 2023

issue that NoWorkResult and LazyResult give different results right?

How we can repeat this issue. I also thought that this issue is only about if check.

@romainmenke
Copy link
Contributor Author

I've added failing tests in a PR : #1909

@romainmenke
Copy link
Contributor Author

romainmenke commented Dec 29, 2023

I think that NoWorkResult doesn't work well and that LazyResult is better.
Modifying the if check so that NoWorkResult is more likely, will make things worse, not better.

I don't currently have the time and capacity to context switch to this issue :/


My current workaround is to always pass a plugin, one that doesn't do anything :

const noopPlugin = () => {
	return {
		postcssPlugin: 'noop-plugin',
		Once() {
			// do nothing
		},
	};
};

noopPlugin.postcss = true;

export default noopPlugin;

This forces postcss to return a LazyResult in all cases.

@ai
Copy link
Member

ai commented Dec 29, 2023

If you have time, can you check why there is a difference between them?

@ahmdammarr
Copy link
Contributor

ahmdammarr commented Dec 30, 2023

@romainmenke as per my understanding the issue here was forcing a LazyResult by mistake!
and my PR solves only this issue

about this👇🏼

Thanks @ahmdammarr,

This does not fix the underlying issue that NoWorkResult and LazyResult give different results right?

If we expect the same result, why would we use two classes NoWorkResults and LazyResult? can't we have just one class (eg:ProccessedResult) that handles these edge cases?

 if (
      this.plugins.length === 0 &&
      typeof opts.parser === 'undefined' &&
      typeof opts.stringifier === 'undefined' &&
      typeof opts.syntax === 'undefined'
    )

@ai
Copy link
Member

ai commented Dec 30, 2023

If we expect the same result, why would we use two classes NoWorkResults and LazyResult? can't we have just one class (eg:ProccessedResult) that handles these edge cases?

Many users are using PostCSS in a wrong way calling it even if they don't need it (they don't use any plugins, etc).

We have NoWorkResult for that case to not call CSS parsing (it takes a lot time) if user don't need PostCSS.

@ahmdammarr
Copy link
Contributor

ahmdammarr commented Dec 30, 2023

If we expect the same result, why would we use two classes NoWorkResults and LazyResult? can't we have just one class (eg:ProccessedResult) that handles these edge cases?

Many users are using PostCSS in a wrong way calling it even if they don't need it (they don't use any plugins, etc).

We have NoWorkResult for that case to not call CSS parsing (it takes a lot time) if user don't need PostCSS.

I did a quick research on how both classes behave, and so far I found that both have lots of common details, so what I'm proposing is to have one class and handle edge cases (eg: not parsing CSS if not needed)

need to spend more time on discovery if what i proposed is valid!

@ai
Copy link
Member

ai commented Dec 30, 2023

Of course we can refactor code, but I prefer to fix them issue first.

@romainmenke
Copy link
Contributor Author

I've opened an issue for more general discussion about NoWorkResult : #1910

@romainmenke
Copy link
Contributor Author

I've added a pull request to address the differences between NoWorkResult and LazyResult : #1909

@ai ai closed this as completed in #1908 Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants