Skip to content

Commit

Permalink
Hot fix for broken isClean on AST dublication
Browse files Browse the repository at this point in the history
  • Loading branch information
ai committed Jun 13, 2021
1 parent 2da5501 commit d8edfed
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 23 deletions.
42 changes: 21 additions & 21 deletions lib/container.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict'

let { isClean, my } = require('./symbols')
let Declaration = require('./declaration')
let { isClean } = require('./symbols')
let Comment = require('./comment')
let Node = require('./node')

Expand All @@ -24,25 +24,6 @@ function markDirtyUp(node) {
}
}

// istanbul ignore next
function rebuild(node) {
if (node.type === 'atrule') {
Object.setPrototypeOf(node, AtRule.prototype)
} else if (node.type === 'rule') {
Object.setPrototypeOf(node, Rule.prototype)
} else if (node.type === 'decl') {
Object.setPrototypeOf(node, Declaration.prototype)
} else if (node.type === 'comment') {
Object.setPrototypeOf(node, Comment.prototype)
}

if (node.nodes) {
node.nodes.forEach(child => {
rebuild(child)
})
}
}

class Container extends Node {
push(child) {
child.parent = this
Expand Down Expand Up @@ -336,7 +317,7 @@ class Container extends Node {

let processed = nodes.map(i => {
// istanbul ignore next
if (typeof i.markDirty !== 'function') rebuild(i)
if (!i[my]) Container.rebuild(i)
i = i.proxyOf
if (i.parent) i.parent.removeChild(i)
if (i[isClean]) markDirtyUp(i)
Expand Down Expand Up @@ -428,3 +409,22 @@ Container.registerAtRule = dependant => {

module.exports = Container
Container.default = Container

// istanbul ignore next
Container.rebuild = node => {
if (node.type === 'atrule') {
Object.setPrototypeOf(node, AtRule.prototype)
} else if (node.type === 'rule') {
Object.setPrototypeOf(node, Rule.prototype)
} else if (node.type === 'decl') {
Object.setPrototypeOf(node, Declaration.prototype)
} else if (node.type === 'comment') {
Object.setPrototypeOf(node, Comment.prototype)
}

if (node.nodes) {
node.nodes.forEach(child => {
Container.rebuild(child)
})
}
}
7 changes: 5 additions & 2 deletions lib/lazy-result.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
'use strict'

let { isClean, my } = require('./symbols')
let MapGenerator = require('./map-generator')
let { isClean } = require('./symbols')
let stringify = require('./stringify')
let Container = require('./container')
let Document = require('./document')
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',
Expand Down Expand Up @@ -134,6 +135,8 @@ class LazyResult {
this.processed = true
this.error = error
}

if (root && !root[my]) Container.rebuild(root)
}

this.result = new Result(processor, root, opts)
Expand Down
2 changes: 2 additions & 0 deletions lib/symbols.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

module.exports.isClean = Symbol('isClean')

module.exports.my = Symbol('my')

This comment has been minimized.

Copy link
@TheSeally

TheSeally Jun 14, 2021

Do we need one more symbol for fix?
I tried to rebuild container by checking isClean symbol and css still processed fine.
Am I missing something or is there a plan for the future? If it's not difficult, can you explain @ai ?

This comment has been minimized.

Copy link
@ai

ai Jun 14, 2021

Author Member

Oops. I missed to set Node[my] = true. Wait a second, I will release 8.3.4.

This comment has been minimized.

Copy link
@TheSeally

TheSeally Jun 14, 2021

But why we can't use symbol isClean to check state of Container/Node?
When I change the symbol from my to isClean in if (root && !root[my]) Container.rebuild(root) and if (!i[my]) Container.rebuild(i) the result is still correct (with nested rules).
I do not want to say that using a new symbol is a mistake, I just want to understand the reason for adding it.
New symbol my will reduce a count of rebuilding or it do something else that I missed?

This comment has been minimized.

Copy link
@ai

ai Jun 14, 2021

Author Member

Fixed 8b4a8b1

Node[my] is used to detect that nodes was created by another PostCSS instance. Every instance will have own my symbol and another AST will not have this flag.

Node[isClean] could be false if nobody touched. So we need more complicated if (isClean in node) which is not readable.

This comment has been minimized.

Copy link
@ai

ai Jun 14, 2021

Author Member

Can you test that postcss-nested bug with postcss 8.3.4?

This comment has been minimized.

Copy link
@ai

ai Jun 14, 2021

Author Member

I pushed new changes to sitnik.ru, so it will be better to go back to old commit, reproduce the bug and then replace lib/* from postcss@8.3.4 files.

This comment has been minimized.

Copy link
@TheSeally

TheSeally Jun 14, 2021

Node[isClean] could be false if nobody touched. So we need more complicated if (isClean in node) which is not readable.

Thanks for explanation. I left out this case

This comment has been minimized.

Copy link
@TheSeally

TheSeally Jun 14, 2021

yeah I will test it

This comment has been minimized.

Copy link
@TheSeally

TheSeally Jun 14, 2021

@ai I checked postcss@8.3.4 with postcss-nested bug. Everything is alright

This comment has been minimized.

Copy link
@ai

ai Jun 14, 2021

Author Member

Thanks. It was really h complex bug, which is difficult to locate

0 comments on commit d8edfed

Please sign in to comment.