Add Document node and related API (#1498) by hudochenkov · Pull Request #1582 · postcss/postcss

hudochenkov · 2021-05-16T18:23:42Z

New Document node and API.

Added Document node.
- Document node is optional. Default PostCSS parser doesn't create it. Because of this in test I had to create node manually every time.
- Added postcss.document() to create node.
- Allowed only Root nodes as children.
- Document node doesn't have raws. Raws are handled in Root nodes. Stringifier concatenates whatever Root stringifier will give. No spacing or anything else is handled.
Allow raws.before on Root node.
Added Document and DocumentExit events to visitors API. They run similarly to Root and RootExit.
Once and OnceExit run once per Root node. So if Document has two Root node children, then Once and OnceExit will run once per root. So in total twice per document.
Added Node#document() method. It works similar to Node#root() — return Document node in a tree, or node itself.
Changed Node#root() to not go higher than the top most parent, which is not Document node.

Closes #1498.

Imaging that we have HTML syntax. Here's input:

<html><style>a{color:black}</style><style>b{z-index:2}</style>

AST will look like:

new Document({
     nodes: [
          new Root({
               raws: {
                    before: "<html><style>",
               },
               nodes: [
                    /* ... */
               ],
          }),
          new Root({
               raws: {
                    before: "</style><style>",
                    after: "</style>",
               },
               nodes: [
                    /* ... */
               ],
          }),
     ],
});

lib/document.d.ts

lib/root.d.ts

test/visitor.test.ts

ai · 2021-05-16T22:18:01Z

It is really great that you thought about many edge cases. Well done PR.

lib/document.d.ts

hudochenkov · 2021-05-17T00:56:53Z

When this PR is ready to merge, maybe Document node should be released as “experimental” or “unstable”? I have plans to create new syntax to parse styled components-like CSS-in-JS. More narrower version of @stylelint/postcss-css-in-js, which will be parsing styled components flavor of CSS-in-JS and works with PostCSS 8. During this field test some problems with Document could arise hypothetically. When everything is good, Document node could be “stable”.

ai · 2021-05-17T01:03:41Z

When this PR is ready to merge, maybe Document node should be released as “experimental” or “unstable”?

Hm. Looks like a good idea, especially since we do not use it in core parser.

Do you know the proper way to do it? Just put a comment in class description?

hudochenkov · 2021-05-17T09:03:36Z

I don't know a proper way. In stylelint we communicated such thing in documentation and in a changelog.

I've added a note to document.d.ts: https://github.com/postcss/postcss/pull/1582/files#diff-e78e125886d3e557b014e52ab18d4041858a7dd675b44989977891cac1253decR16

lib/document.d.ts

lib/document.js

lib/postcss.d.ts

hudochenkov · 2021-05-17T16:25:09Z

I've addressed all comments.

lib/at-rule.d.ts

ai · 2021-05-17T21:23:35Z

lib/declaration.d.ts

 */
 export default class Declaration extends Node {
  type: 'decl'
+  parent: Container | undefined


Should we move it to Node generic too? And use Node<Parent extends Node = Container>, Container<Child …, Parent …> extends Node<Parent>

I have no luck with this due lack of experience working with generics and classes. When added <Parent extends Node = Container> to Node class, then visitor test shows type errors. For Container I couldn't get past error in Container type.

Don’t worry. I will try to fix it after merging PR.

ai · 2021-05-17T21:24:47Z

lib/document.d.ts

+ * document.nodes.length //=> 2
+ * ```
+ */
+export default class Document extends Container<Root> {


If Document has no raws, how we will track symbols after the last style?

Should we add Document.raws.after? It will fit AtRule and Rule way to keep last symbols.

I have in mind that Root.raws.before and Root.raws.after are used:

postcss/test/stringifier.test.js

Lines 266 to 286 in a5ac723

it('handles document with three roots, with before and after raws', () => {

let root1 = new Root({ raws: { before: 'BEFORE_ONE', after: 'AFTER_ONE' } })

root1.append(new Rule({ selector: 'a.one' }))

let root2 = new Root({ raws: { after: 'AFTER_TWO' } })

root2.append(new Rule({ selector: 'a.two' }))

let root3 = new Root({ raws: { after: 'AFTER_THREE' } })

root3.append(new Rule({ selector: 'a.three' }))

let document = new Document()

document.append(root1)

document.append(root2)

document.append(root3)

let s = document.toString()

expect(s).toEqual(

'BEFORE_ONEa.one {}AFTER_ONEa.two {}AFTER_TWOa.three {}AFTER_THREE'

)

})

Hm. I am worried that it is a different way to take deal with spaces compared to Rule/AtRule.

But on another hand, we already have Root.raws.after.

Do you think that the Root.raws.after is better?

Current state

Rule/AtRule treat before as string before the rule. after is a string after the last child node.

Root doesn't used before in default parser. after is a string after the last child node.

Name after is confusing, because it doesn't do opposite of before. It should have been afterLastChild :)

First idea

I think if we want to keep the same approach to before (string before the node) and after (string after the last child of the node), we would need to add Root.raws.before, and Document.raws.after.

Root.raws.before is already added in this PR, Document.raws.after is missing.

For the following HTML:

<html> <head> <style> a{color:black} </style> <style> b{z-index:2} </style> </head>

We would have following AST:

new Document({ raws: { after: "</style>\n</head>", }, nodes: [ new Root({ raws: { before: "<html>\n<head>\n<style>", after: "\n\n", }, nodes: [ /* ... */ ], }), new Root({ raws: { before: "</style>\n<style>", after: "\n", }, nodes: [ /* ... */ ], }), ], });

In this examples Root boundaries would be content between style tags. As if we took this content and put it in a separate CSS file and parsed.

Second idea (a better one, in my opinion)

After writing above, looking at the AST, and some thinking... I think this is all wrong :)

postcss/lib/node.d.ts

Lines 144 to 150 in ab60e0d

* Every parser saves its own properties,

* but the default CSS parser uses:

*

* * `before`: the space symbols before the node. It also stores `*`

* and `_` symbols before the declaration (IE hack).

* * `after`: the space symbols after the last child of the node

* to the end of the node.

Adding Root.raws.before and using it not for spacing could potentially be a breaking change. E. g. some plugin looks into every node's raws.before and remove spaces. Then if before for parsed HTML would contain some HTML markup stringified output would be broken.

This is what I think is what we need to have. Due to differences what parsed document could be (HTML, JS, TS, Markdown, vue, etc) we should not change any raws. So revert Root.raws.before in this PR.

Document.raws should be in types, but no specifics (any).

Every syntax will put whatever they want in Document.raws, and in Root could use any new key.

Idea is to keep Root the same as if PostCSS default parser would create it, but we could add new raws keys with names, that PostCSS doesn't use.

Here how I would make HTML parser:

new Document({ nodes: [ new Root({ raws: { markupBefore: "<html>\n<head>\n<style>", after: "\n\n", }, nodes: [ /* ... */ ], }), new Root({ raws: { markupBefore: "</style>\n<style>", after: "\n", markupAfter: "</style>\n</head>", }, nodes: [ /* ... */ ], }), ], });

If we remove markupBefore and markupAfter it would be regular CSS and any plugin will work as expected.

So custom syntax will create these new raws and then used them for stringifying.

After writing above, looking at the AST, and some thinking... I think this is all wrong :)

Is it about first or second idea?

Second idea (a better one, in my opinion)

I like it more, but with codeBefore and codeAfter instead of markup, since we will not have markup in CSS-in-JS.

I think first idea is a bad one.

In second idea raws names are up to developer of a syntax, because PostCSS wouldn't do anything with it. But I think your names for raws are better :)

Good. Change the raws and we are ready to merge.

I removed Root.raws.before

Added raws for Document and decided to test custom syntax with Document. This revealed that some types need to be adjusted. Also it helpful to have at least one test with syntax which works with Document :)

lib/node.js

ai · 2021-05-19T00:51:17Z

I renamed markupBefore to codeBefore and added them to Root.raws types 7b3c872

ai · 2021-05-19T00:53:30Z

I will try to release 8.3 tomorrow

hudochenkov · 2021-05-19T08:15:12Z

I renamed markupBefore to codeBefore

Damn it, I wanted to use new names but was caught up in understanding how stringifier works :)

Thank you!

ai · 2021-05-21T03:42:44Z

Released in 8.3.

hudochenkov · 2021-05-21T08:21:29Z

@ai Thank you!

The introduction of the `Document` type [in this PR](postcss/postcss#1582) incorrectly advertises that `parse` can return a `Document` object, because the [`Parser` interface is now generic](https://github.com/postcss/postcss/pull/1582/files#diff-68ba6abc949516587990ca61016794e81039fd6ae58437d5cfaccd4eeebdac07R224). This PR exposes the parser as a more specific type of `Parser` so that consumers aren't given a `Root | Document`, but just a `Root`.

Add Document node and related API (postcss#1498)

eb896ce

hudochenkov commented May 16, 2021

View reviewed changes

lib/document.d.ts Outdated Show resolved Hide resolved

lib/root.d.ts Outdated Show resolved Hide resolved

test/visitor.test.ts Show resolved Hide resolved

test/visitor.test.ts Show resolved Hide resolved

test/visitor.test.ts Show resolved Hide resolved

hudochenkov marked this pull request as ready for review May 16, 2021 18:31

hudochenkov added 2 commits May 17, 2021 02:38

Narrow Document nodes type

aa25ce0

Specify parent type on a Root node

39d2a84

hudochenkov commented May 17, 2021

View reviewed changes

lib/document.d.ts Outdated Show resolved Hide resolved

hudochenkov added 2 commits May 17, 2021 10:56

Fix types

fca648b

Add experimental warning

da90932