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 Document node and related API (#1498) #1582
Conversation
It is really great that you thought about many edge cases. Well done PR. |
When this PR is ready to merge, maybe |
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? |
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 |
I've addressed all comments. |
@@ -51,6 +52,7 @@ export interface DeclarationProps { | |||
*/ | |||
export default class Declaration extends Node { | |||
type: 'decl' | |||
parent: Container | undefined |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* document.nodes.length //=> 2 | ||
* ``` | ||
*/ | ||
export default class Document extends Container<Root> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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' | |
) | |
}) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
:)
I renamed |
I will try to release 8.3 tomorrow |
Damn it, I wanted to use new names but was caught up in understanding how stringifier works :) Thank you! |
Released in 8.3. |
@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`.
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`.
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`.
New
Document
node and API.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.postcss.document()
to create node.Root
nodes as children.Document
node doesn't haveraws
. Raws are handled inRoot
nodes. Stringifier concatenates whateverRoot
stringifier will give. No spacing or anything else is handled.raws.before
onRoot
node.Document
andDocumentExit
events to visitors API. They run similarly toRoot
andRootExit
.Once
andOnceExit
run once perRoot
node. So ifDocument
has twoRoot
node children, thenOnce
andOnceExit
will run once per root. So in total twice per document.Node#document()
method. It works similar toNode#root()
— returnDocument
node in a tree, or node itself.Node#root()
to not go higher than the top most parent, which is notDocument
node.Closes #1498.
Imaging that we have HTML syntax. Here's input:
AST will look like: