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

postcss 8.4.48 probably broke stylelint-stylus, @stylistic/stylelint-plugin and stylelint-plugin-stylus tests #62

Open
Mouvedia opened this issue Nov 30, 2024 · 9 comments

Comments

@Mouvedia Mouvedia changed the title postcss 8.4.48 probably broke stylelint-stylus test postcss 8.4.48 probably broke stylelint-stylus, @stylistic/stylelint-plugin and stylelint-plugin-styulus tests Nov 30, 2024
@ybiquitous
Copy link
Member

ybiquitous commented Dec 7, 2024

I could reproduce an issue in the @stylistic/stylelint-plugin test suite with postcss-html:

		{
			code: `<div>
<style>a {
  color: red;
}</style>


</div>`,
			message: messages.rejected,
			line: 4,
			column: 1,
		},

https://github.com/stylelint-stylistic/stylelint-stylistic/blob/32cff1c6107ca12bd149c81727fd6bc1377d3853/lib/rules/no-missing-end-of-source-newline/index.test.js#L95-L105

We can also see this issue on our demo site as follows.

With [email protected], the lint problem's location is 3:2-3. See demo.

Image

With [email protected], the lint problem's location is 2:9-10. See demo.

Image

The difference between the two demos is only postcss versions.

I suppose that this commit (postcss/postcss@1a8b261) for rangeBy() is related, but I'm not still sure which package we should fix, e.g. postcss, postcss-html, @stylistic/stylelint-plugin, or stylelint. 🤔

@romainmenke If you have any ideas, please feel free to let us know. 🙏🏼

@Mouvedia
Copy link
Member Author

Mouvedia commented Dec 7, 2024

but I'm not still sure which package we should fix

You could say that it should have been a minor version.
i.e. 8.4.47 → 8.5.0
But that's irrelevant now, if postcss consider it a fix it is a patch.

tl;dr: I have opened this issue so that we can track the issues on @stylistic/stylelint-plugin and stylelint-stylus

@Mouvedia Mouvedia changed the title postcss 8.4.48 probably broke stylelint-stylus, @stylistic/stylelint-plugin and stylelint-plugin-styulus tests postcss 8.4.48 probably broke stylelint-stylus, @stylistic/stylelint-plugin and stylelint-plugin-stylus tests Dec 7, 2024
@romainmenke
Copy link
Member

romainmenke commented Dec 7, 2024

PostCSS is large enough that any change is a breaking change for someone.
This type of breakage was expected with those changes.


If I understand the issue correctly then this is roughly what is happening.

postcss-html (and likely similar packages) have node.source.input.css as the content of <style>...</style> block while also having node.source.start relative to the whole document.

We can't have both, those are contradictory.
This is best reported upstream in affected packages so that their respective communities can provide patches.

postcss-html and similar packages use the Document node to represent multiple Root nodes found in a non-css file. Like html, jsx, ... So there needs to be some kind of mapping between both coordinate systems.

With the patches in PostCSS the workarounds used by postcss-html and others likely became incorrect.

A minimal repro for postcss-html:

const assert = require('assert');
const syntax = require("./lib/index.js")();
const document = syntax
	.parse(`<div>
<style>
a {
  color: red;
}</style>

</div>`, { syntax, from: 'foo' });

assert.deepStrictEqual(
	document.first.rangeBy({ index: document.first.source.input.css.length - 1 }),
	{ end: { column: 2, line: 5 }, start: { column: 1, line: 5 } }
);

I don't currently have much time and this is unlikely to improve over the next few months.
So any help with reporting this upstream would be greatly appreciated 🙇

@Mouvedia
Copy link
Member Author

Mouvedia commented Dec 7, 2024

@firefoxic @ota-meshi

@ybiquitous
Copy link
Member

Thank you for your help. Let me investigate it more.
Once I identify the cause, I'll open an issue upstream. 👍🏼

@romainmenke
Copy link
Member

I've also filed an issue in PostCSS as I think we can make a small change there to make it easier for postcss-html and other to update their code:

postcss/postcss#1995

@Mouvedia
Copy link
Member Author

@ai @ajcwebdev
see postcss/postcss#1996

@ai
Copy link
Member

ai commented Jan 13, 2025

I am going to release it today =^_^=

@Mouvedia
Copy link
Member Author

Mouvedia commented Jan 19, 2025

@firefoxic we are down from 2 tests failing to 1.
i.e. 14/01/25 vs 15/01/25

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

No branches or pull requests

4 participants