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

Add new Document node type #1498

Closed
hudochenkov opened this issue Jan 2, 2021 · 7 comments · Fixed by #1582
Closed

Add new Document node type #1498

hudochenkov opened this issue Jan 2, 2021 · 7 comments · Fixed by #1582

Comments

@hudochenkov
Copy link
Contributor

It's a lot of text, I wanted to include enough information. It is important for a front-end tooling ecosystem. But maybe it's just a stylelint only problem with custom syntaxes ¯\_(ツ)_/¯

Custom syntaxes

PostCSS by default supports CSS in a form, where parsed file has only CSS. Most PostCSS syntaxes also for more classic CSS usage: SCSS, Sass, SugarSS, Less. However there are cases, when CSS is a part of other language. style tags and style attributes in HTML, Vue, Svelte. CSS-in-JS in JS template literals (Styled Components) or objects (JSS).

Currently stylelint supports both types of files: classic one file — one syntax, and the other one file — multiple syntaxes. It is done by huge work of @gucong3000. He made custom syntaxes to support multiple syntaxes in one file:

All this syntaxes are powered by postcss-syntax. This package does two jobs: automatically switch PostCSS syntax based on file extensions, and provides common tools for other syntaxes (more on this later).

These packages were developed only by him. Unfortunately he is not active on Github for the past two years. These packages are complex and intertwined together. It's hard to understand how they work. No one else know how they work. I digged into them recently in an attempt to upgrade to PostCSS 8. Here's my surface understanding (they my not be accurate).

postcss-syntax

postcss-syntax does two jobs. First job is documented in package's readme: automatically switch PostCSS syntax based on file extensions. Second job is not documented, but very important for all other syntaxes: provide common tools for syntaxes. It introduces a new AST node type Document, but new AST node still has type: 'root', because it's extended from PostCSS Root class. It does it by monkey-patching PostCSS via Node.js require.cache (crazy hack). For example for HTML document with two style tags we would have this kind of AST:

Document {
	nodes: [
		Root {
			nodes: [
				Rule {},
				AtRule {},
			]
		},
		Root {
			nodes: [
				Rule {},
				AtRule {},
			]
		},
	]
}

postcss-syntax makes sure that every PostCSS plugins are run on each Root.

Also it adds extra information to sources. Probably, for a successful stringifying.

Another issues with these syntaxes:

  • Implicit dependencies and code execution. postcss-syntax also calls custom syntax internal files, when postcss/syntax/load-syntax.js is called from custom syntax.
  • They use postcss internal files, which I believe is not allowed, because they are not part of official API.

Why it works this way

I don't know exactly why postcss-syntax and dependent syntaxes architected this way.

However, I have an idea about some of behavior of these custom syntaxes. Each stylelint rule is a PostCSS plugin. These syntaxes were created with stylelint in mind. For many stylelint rules it makes more sense to have e. g. every styled component (const Element = styled.div``) as a separate root so a rule (PostCSS plugin) is run only on this piece of code.

For example we have following HTML:

<p style="color: red"></p>

<p style="color: blue"></p>

If we don't distinguish them as separate Roots, rule like declaration-block-no-duplicate-properties would show a lint violation.

We can't use Rule type for each style attribute, because we have many lint rules, which would check different aspects of Rule nodes (selectors, brackets, spacings), which would not be applicable for this fake Rule. In the mean time there is no stylelint rules, which has any specific behavior for a Root node.

Having postcss-syntax parse each entity as a separate Root, and they run plugins over each Root, allowed adoption of all this syntaxes in stylelint without changing hundreds of rules in stylelint codebase and in stylelint plugins.

What it's all about

Custom syntaxes supported by stylelint and its plugins were in a rough state for the past two years. And usage of CSS in files, which are not CSS only, is increasing. It's important for stylelint and PostCSS to support such cases. I would like to find a solution to this problem. postcss-css-in-js, postcss-html, postcss-markdown and postcss-syntax in they current form should be changed. To support PostCSS 8 and unblock community to make changes to this syntaxes, by refactoring and making code architecture more clear.

I think it's important to find a vector which PostCSS and custom syntaxes should follow to support all types of CSS flavors modern development has.

@ai
Copy link
Member

ai commented Jan 2, 2021

How do you see PostCSS AST changes?

@hudochenkov
Copy link
Contributor Author

Maybe we can have an official Document AST node type. It would have only Root as children nodes. PostCSS then will run Root and Once methods over these child Root, but not on parent Document node. Maybe we would need Document method, or even DocumentOnce.

@ai
Copy link
Member

ai commented Jan 2, 2021

I am OK with PR adding the Document nodes type. We may need a few small stringifier fixes. Document event is also OK, but I do not think we need a DocumentOnce.

Does somebody have access to postcss-syntax to fix it?

@hudochenkov
Copy link
Contributor Author

hudochenkov commented Jan 3, 2021

I am OK with PR adding the Document nodes type. We may need a few small stringifier fixes. Document event is also OK, but I do not think we need a DocumentOnce.

I'm going to try to do this.

  • Do you have some guidelines what needs to be done?
  • Shall we have Document node present in all AST PostCSS makes? Like all AST has Root, but now all AST would also have Document. Or Document would be optional. Some syntaxes might not have it, some syntaxes could have it.
  • Shall we have some properties from Root moved to Document?
  • Do you have in mind what properties Document should have? Maybe something new, that no other node has.
  • Shall we somehow limit that Document could only have Root as direct children nodes?

Does somebody have access to postcss-syntax to fix it?

No one has access to original repositories. We moved postcss-markdown and postcss-css-in-js to stylelint organization and release under @stylelint namespace some time ago. And recently we moved postcss-html and postcss-syntax to our organization to maintain them. We will release them under @stylelint namespace as well.

@hudochenkov hudochenkov changed the title Multi Root ASTs Add new Document node type Jan 3, 2021
@ai
Copy link
Member

ai commented Jan 3, 2021

Do you have some guidelines what needs to be done?

  1. Add class.
  2. Add a full integration test that everything is working.
  3. Double-check that the stringifier is working.
  4. Add TypeDoc with an explanation for Document use cases and examples.
  5. Add Document and DocumentExit events

Shall we have Document node present in all AST PostCSS makes? Like all AST has Root, but now all AST would also have Document. Or Document would be optional.

I think it should be optional since it is a rare case.

We should not make AST more complicated for this rare case.

Shall we have some properties from Root moved to Document?

I plan to release it as 8.2, so I prefer to avoid breaking changes. The community is still mad at me because of 7→ 8 migration 😅.

Am I right that JS code from CSS-in-JS files we keep in Root#raws.before and Document#raws.after?

Do you have in mind what properties Document should have?

I am thinking about Document.input

Shall we somehow limit that Document could only have Root as direct children nodes?

Yeap, let’s limit it in TS types.

@jeddy3
Copy link

jeddy3 commented Jan 3, 2021

This is exciting stuff! I had one minor point to chime in on.

And recently we moved postcss-html and postcss-syntax to our organization to maintain them. We will release them under @stylelint namespace as well.

Like postcss-js and postcss-scss, it'd be great if these syntaxes lived in the PostCSS org (and were published outside of the stylelint NPM scope). These syntaxes are applicable to more than stylelint, e.g. #1494 which was for adding Tailwind to LitElement Web Components.

I was quickly digging into the process for NPM disputes and it turns out you already have ownership, @ai.

jeddy3@mac % npm owner ls postcss-syntax
ai <andrey@sitnik.ru>
gucong <gucong@gmail.com>
jeddy3@mac % npm owner ls postcss-jsx     
ai <andrey@sitnik.ru>
gucong <gucong@gmail.com>
jeddy3@mac % npm owner ls postcss-markdown
ai <andrey@sitnik.ru>
gucong <gucong@gmail.com>
jeddy3@mac % npm owner ls postcss-html    
ai <andrey@sitnik.ru>
gucong <gucong@gmail.com>

In hindsight, I should've checked this before we published our forks under the stylelint NPM scope a while back.

It's a minor thing, but it'd be nice to tidy this up as I think it'd be healthy for the PostCSS ecosystem.

@ai
Copy link
Member

ai commented Jan 3, 2021

@jeddy3 I do not have time to support these syntaxes. But we can move them to postcss org if the Stylelint team is OK with it.

hudochenkov added a commit to hudochenkov/postcss that referenced this issue May 16, 2021
hudochenkov added a commit to hudochenkov/postcss that referenced this issue May 16, 2021
hudochenkov added a commit to hudochenkov/postcss that referenced this issue May 16, 2021
ai added a commit that referenced this issue May 19, 2021
@ai ai closed this as completed May 19, 2021
ai pushed a commit to postcss/postcss-scss that referenced this issue Jan 10, 2022
The introduction of the `Document` type [in this PR](postcss/postcss#1498) incorrectly advertises that `parse` can return a `Document` object.
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 a pull request may close this issue.

3 participants