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.

Sign up for GitHub

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

Jump to bottom

Add Document node and related API (#1498) #1582

Merged
merged 12 commits into from May 19, 2021
Merged

Add Document node and related API (#1498) #1582

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eb896ce
Add Document node and related API (#1498)
hudochenkov May 16, 2021
aa25ce0
Narrow Document nodes type
hudochenkov May 16, 2021
39d2a84
Specify parent type on a Root node
hudochenkov May 17, 2021
fca648b
Fix types
hudochenkov May 17, 2021
da90932
Add experimental warning
hudochenkov May 17, 2021
a5ac723
Fix types
hudochenkov May 17, 2021
69c5d81
Remove unneeded parent type
hudochenkov May 17, 2021
50e07d1
Remove `Node#document()`
hudochenkov May 18, 2021
49eec3a
Remove Root.raws.before
hudochenkov May 18, 2021
54e2e52
Test custom parser
hudochenkov May 18, 2021
3eb068b
Fix Parser type
hudochenkov May 18, 2021
8af26ab
Update documentation
hudochenkov May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/at-rule.d.ts
View file
Open in desktop
@@ -1,4 +1,5 @@
import Container, { ContainerProps } from './container.js'
import { ChildNode } from './node.js'

interface AtRuleRaws {
/**
Expand Down Expand Up @@ -69,7 +70,9 @@ export interface AtRuleProps extends ContainerProps {
*/
export default class AtRule extends Container {
type: 'atrule'
parent: Container | undefined
raws: AtRuleRaws
nodes: ChildNode[]
ai marked this conversation as resolved.
Show resolved Hide resolved

/**
* The at-rule’s name immediately follows the `@`.
Expand Down
2 changes: 2 additions & 0 deletions lib/comment.d.ts
View file
Open in desktop
@@ -1,3 +1,4 @@
import Container from './container.js'
import Node, { NodeProps } from './node.js'

interface CommentRaws {
Expand Down Expand Up @@ -37,6 +38,7 @@ export interface CommentProps extends NodeProps {
*/
export default class Comment extends Node {
type: 'comment'
parent: Container | undefined
raws: CommentRaws

/**
Expand Down
42 changes: 16 additions & 26 deletions lib/container.d.ts
View file
Open in desktop
Expand Up @@ -27,7 +27,9 @@ export interface ContainerProps extends NodeProps {
* Note that all containers can store any content. If you write a rule inside
* a rule, PostCSS will parse it.
*/
export default abstract class Container extends Node {
export default abstract class Container<
Child extends Node = ChildNode
> extends Node {
/**
* An array containing the container’s children.
*
Expand All @@ -38,7 +40,7 @@ export default abstract class Container extends Node {
* root.nodes[0].nodes[0].prop //=> 'color'
* ```
*/
nodes: ChildNode[]
nodes: Child[]

/**
* The container’s first child.
Expand All @@ -47,7 +49,7 @@ export default abstract class Container extends Node {
* rule.first === rules.nodes[0]
* ```
*/
get first(): ChildNode | undefined
get first(): Child | undefined

/**
* The container’s last child.
Expand All @@ -56,7 +58,7 @@ export default abstract class Container extends Node {
* rule.last === rule.nodes[rule.nodes.length - 1]
* ```
*/
get last(): ChildNode | undefined
get last(): Child | undefined

/**
* Iterates through the container’s immediate children,
Expand Down Expand Up @@ -92,7 +94,7 @@ export default abstract class Container extends Node {
* @return Returns `false` if iteration was broke.
*/
each(
callback: (node: ChildNode, index: number) => false | void
callback: (node: Child, index: number) => false | void
): false | undefined

/**
Expand Down Expand Up @@ -304,7 +306,7 @@ export default abstract class Container extends Node {
* @param child New node.
* @return This node for methods chain.
*/
push(child: ChildNode): this
push(child: Child): this

/**
* Insert new node before old node within the container.
Expand All @@ -318,14 +320,8 @@ export default abstract class Container extends Node {
* @return This node for methods chain.
*/
insertBefore(
oldNode: ChildNode | number,
newNode:
| ChildNode
| ChildProps
| string
| ChildNode[]
| ChildProps[]
| string[]
oldNode: Child | number,
newNode: Child | ChildProps | string | Child[] | ChildProps[] | string[]
): this

/**
Expand All @@ -336,14 +332,8 @@ export default abstract class Container extends Node {
* @return This node for methods chain.
*/
insertAfter(
oldNode: ChildNode | number,
newNode:
| ChildNode
| ChildProps
| string
| ChildNode[]
| ChildProps[]
| string[]
oldNode: Child | number,
newNode: Child | ChildProps | string | Child[] | ChildProps[] | string[]
): this

/**
Expand All @@ -360,7 +350,7 @@ export default abstract class Container extends Node {
* @param child Child or child’s index.
* @return This node for methods chain.
*/
removeChild(child: ChildNode | number): this
removeChild(child: Child | number): this

/**
* Removes all children from the container
Expand Down Expand Up @@ -420,7 +410,7 @@ export default abstract class Container extends Node {
* @return Is every child pass condition.
*/
every(
condition: (node: ChildNode, index: number, nodes: ChildNode[]) => boolean
condition: (node: Child, index: number, nodes: Child[]) => boolean
): boolean

/**
Expand All @@ -435,7 +425,7 @@ export default abstract class Container extends Node {
* @return Is some child pass condition.
*/
some(
condition: (node: ChildNode, index: number, nodes: ChildNode[]) => boolean
condition: (node: Child, index: number, nodes: Child[]) => boolean
): boolean

/**
Expand All @@ -448,5 +438,5 @@ export default abstract class Container extends Node {
* @param child Child of the current container.
* @return Child index.
*/
index(child: ChildNode | number): number
index(child: Child | number): number
}
2 changes: 1 addition & 1 deletion lib/container.js
View file
Open in desktop
Expand Up @@ -310,7 +310,7 @@ class Container extends Node {
for (let i of nodes) {
if (i.parent) i.parent.removeChild(i, 'ignore')
}
} else if (nodes.type === 'root') {
} else if (nodes.type === 'root' && this.type !== 'document') {
nodes = nodes.nodes.slice(0)
for (let i of nodes) {
if (i.parent) i.parent.removeChild(i, 'ignore')
Expand Down
2 changes: 2 additions & 0 deletions lib/declaration.d.ts
View file
Open in desktop
@@ -1,3 +1,4 @@
import Container from './container.js'
import Node from './node.js'

interface DeclarationRaws {
Expand Down Expand Up @@ -51,6 +52,7 @@ export interface DeclarationProps {
*/
export default class Declaration extends Node {
type: 'decl'
parent: Container | undefined
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

raws: DeclarationRaws

/**
Expand Down
46 changes: 46 additions & 0 deletions lib/document.d.ts
View file
Open in desktop
@@ -0,0 +1,46 @@
import Container, { ContainerProps } from './container.js'
import { ProcessOptions } from './postcss.js'
import Result from './result.js'
import Root, { RootProps } from './root.js'

export interface DocumentProps extends ContainerProps {
nodes?: Root[]
}

type ChildNode = Root
type ChildProps = RootProps

/**
* Represents a file and contains all its parsed nodes.
*
* **Experimental:** some aspects of this node could change within minor or patch version releases.
*
* ```js
* const document = htmlParser('<html><style>a{color:black}</style><style>b{z-index:2}</style>')
* document.type //=> 'document'
* document.nodes.length //=> 2
* ```
*/
export default class Document extends Container<Root> {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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'
)
})

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

type: 'document'
parent: undefined

constructor(defaults?: DocumentProps)

/**
* Returns a `Result` instance representing the document’s CSS roots.
*
* ```js
* const root1 = postcss.parse(css1, { from: 'a.css' })
* const root2 = postcss.parse(css2, { from: 'b.css' })
* const document = postcss.document()
* document.append(root1)
* document.append(root2)
* const result = document.toResult({ to: 'all.css', map: true })
* ```
*
* @param opts Options.
* @return Result with current document’s CSS.
*/
toResult(options?: ProcessOptions): Result
}
33 changes: 33 additions & 0 deletions lib/document.js
View file
Open in desktop
@@ -0,0 +1,33 @@
'use strict'

let Container = require('./container')

let LazyResult, Processor

class Document extends Container {
ai marked this conversation as resolved.
Show resolved Hide resolved
constructor(defaults) {
// type needs to be passed to super, otherwise child roots won't be normalized correctly
super({ type: 'document', ...defaults })

if (!this.nodes) {
this.nodes = []
}
}

toResult(opts = {}) {
let lazy = new LazyResult(new Processor(), this, opts)

return lazy.stringify()
}
}

Document.registerLazyResult = dependant => {
LazyResult = dependant
}

Document.registerProcessor = dependant => {
Processor = dependant
}

module.exports = Document
Document.default = Document
51 changes: 45 additions & 6 deletions lib/lazy-result.js
View file
Open in desktop
Expand Up @@ -7,8 +7,10 @@ let warnOnce = require('./warn-once')
let Result = require('./result')
let parse = require('./parse')
let Root = require('./root')
let Document = require('./document')

const TYPE_TO_CLASS_NAME = {
document: 'Document',
root: 'Root',
atrule: 'AtRule',
rule: 'Rule',
Expand All @@ -20,6 +22,7 @@ const PLUGIN_PROPS = {
postcssPlugin: true,
prepare: true,
Once: true,
Document: true,
Root: true,
Declaration: true,
Rule: true,
Expand All @@ -30,6 +33,7 @@ const PLUGIN_PROPS = {
AtRuleExit: true,
CommentExit: true,
RootExit: true,
DocumentExit: true,
OnceExit: true
}

Expand Down Expand Up @@ -73,7 +77,9 @@ function getEvents(node) {

function toStack(node) {
let events
if (node.type === 'root') {
if (node.type === 'document') {
events = ['Document', CHILDREN, 'DocumentExit']
} else if (node.type === 'root') {
events = ['Root', CHILDREN, 'RootExit']
} else {
events = getEvents(node)
Expand Down Expand Up @@ -103,7 +109,11 @@ class LazyResult {
this.processed = false

let root
if (typeof css === 'object' && css !== null && css.type === 'root') {
if (
typeof css === 'object' &&
css !== null &&
(css.type === 'root' || css.type === 'document')
) {
root = cleanMarks(css)
} else if (css instanceof LazyResult || css instanceof Result) {
root = cleanMarks(css.root)
Expand Down Expand Up @@ -231,7 +241,13 @@ class LazyResult {
this.walkSync(root)
}
if (this.listeners.OnceExit) {
this.visitSync(this.listeners.OnceExit, root)
if (root.type === 'document') {
for (let subRoot of root.nodes) {
this.visitSync(this.listeners.OnceExit, subRoot)
}
} else {
this.visitSync(this.listeners.OnceExit, root)
}
}
}

Expand Down Expand Up @@ -287,7 +303,9 @@ class LazyResult {
} catch (e) {
throw this.handleError(e, node.proxyOf)
}
if (node.type !== 'root' && !node.parent) return true
if (node.type !== 'root' && node.type !== 'document' && !node.parent) {
return true
}
if (isPromise(promise)) {
throw this.getAsyncError()
}
Expand All @@ -298,6 +316,18 @@ class LazyResult {
this.result.lastPlugin = plugin
try {
if (typeof plugin === 'object' && plugin.Once) {
if (this.result.root.type === 'document') {
let roots = this.result.root.nodes.map(root =>
plugin.Once(root, this.helpers)
)

if (isPromise(roots[0])) {
return Promise.all(roots)
}

return roots
}

return plugin.Once(this.result.root, this.helpers)
} else if (typeof plugin === 'function') {
return plugin(this.result.root, this.result)
Expand Down Expand Up @@ -385,7 +415,15 @@ class LazyResult {
for (let [plugin, visitor] of this.listeners.OnceExit) {
this.result.lastPlugin = plugin
try {
await visitor(root, this.helpers)
if (root.type === 'document') {
let roots = root.nodes.map(subRoot =>
visitor(subRoot, this.helpers)
)

await Promise.all(roots)
} else {
await visitor(root, this.helpers)
}
} catch (e) {
throw this.handleError(e)
}
Expand Down Expand Up @@ -439,7 +477,7 @@ class LazyResult {
let visit = stack[stack.length - 1]
let { node, visitors } = visit

if (node.type !== 'root' && !node.parent) {
if (node.type !== 'root' && node.type !== 'document' && !node.parent) {
stack.pop()
return
}
Expand Down Expand Up @@ -501,3 +539,4 @@ module.exports = LazyResult
LazyResult.default = LazyResult

Root.registerLazyResult(LazyResult)
Document.registerLazyResult(LazyResult)