Repository: shellscape/postcss-less
Pull request #82 (2017-07-01 10:29:50)

Supply a Root stringifier
**Which issue #** if any, does this resolve?
No issue, see #81 for some related discussion.

<!-- PRs must be accompanied by related tests -->

I'm not really sure how to test this, if I come up with something I'll complete this PR with it.

Please check one:
- [ ] New tests created for this change
- [ ] Tests updated for this change

This PR:
- [ ] Adds new API
- [x] Extends existing API, backwards-compatible
- [ ] Introduces a breaking change
- [ ] Fixes a bug

---

I discovered that stringifying the `Root` node when it contains `Import` nodes would cause problems if the `postcss-less` stringifier wasn't supplied. However, it's not always possible to pass the stringifier and thus, the stringifying would fail.

Creating our own `Root` node with a stringifier (extending from the regular `postcss` `Root` node) we can supply our stringifier straight from the start and it'll always be used without the caller having to do anything.


shellscape: @jwilsson code looks good. But I'd like to get some more documentation in the PR for future reference. Some "before" code blocks that demonstrate the problem, and maybe the situation in which we can't pass a stringifier, and some "after" code that shows the fix in action. (this will probably need a blog post from me to explain the change, for which that stuff will be immensely helpful). I haven't seen the situation that this fixes, so that'd be helpful to conceptualize the problem as well.
jwilsson: @shellscape Sure thing!

In `postcss`, there's a very handy method called `Node.positionBy()` which gives you the line/column position of a string somewhere inside that node. Internally it's a pretty simple string search but in order to get the string it'll of course need to stringify the passed node (`root` for example) and all of its children. This is where the problem arises, because earlier the `Root` node would be the one from `postcss` which only used the standard stringifier which doesn't support `Import` nodes. And there's no way to pass another stringifier to `positionBy()`. Hence this change, which makes sure the `Root` node is always created by `postcss-less` and exposing its own stringifier.

It worked earlier when called directly on child nodes to `Root` since they're already emitted as `Rule` nodes which are created by `postcss-less` and thus exposing its own stringifier.

Minimum example demostrating the problem (even when passing the custom stringifier):

```js
const postcss = require('postcss');
const postcssLess = require('postcss-less');
const stringify = require('postcss-less/dist/less-stringify');

const lessCode = '@import "foo.less"';

postcss().process(lessCode, {
    syntax: postcssLess,
    stringifier: stringify,
})
.then((result) => {
    console.log(result.root.positionBy({
        word: 'foo',
    }));
}).catch((error) => {
    console.error(error);
});
```

I hope this makes sense!
jwilsson: `Node.positionBy()` is of course just an example, there are probably other methods that will fail in similar ways. Even something as simple as this will fail:

```js
const postcss = require('postcss');
const postcssLess = require('postcss-less');
const stringify = require('postcss-less/dist/less-stringify');

const lessCode = '@import "foo.less"';

postcss().process(lessCode, {
    syntax: postcssLess,
    stringifier: stringify,
})
.then((result) => {
    console.log(result.root.toString()); // Just try to stringify it
}).catch((error) => {
    console.error(error);
});
```

But that can be fixed by passing the stringifier to `toString()`:

```js
const postcss = require('postcss');
const postcssLess = require('postcss-less');
const stringify = require('postcss-less/dist/less-stringify');

const lessCode = '@import "foo.less"';

postcss().process(lessCode, {
    syntax: postcssLess,
    stringifier: stringify,
})
.then((result) => {
    console.log(result.root.toString({
        stringify: stringify, // Note this change here. It'll will work without this using this PR though
    }));
}).catch((error) => {
    console.error(error);
});

```

How pretty/ugly that is, I'll leave unsaid but it won't be needed given the code in this PR.

I should've made this more clear in my previous post, sorry.
shellscape: ![mind blown](https://media.giphy.com/media/OK27wINdQS5YQ/giphy.gif)

Amazing detective work, sir. The explanation and fix make total sense. Thanks for all of that.
shellscape: > I'm not really sure how to test this, if I come up with something I'll complete this PR with it.

I think using the Node.positionBy method would be a fine test of this. If you'd rather I added that after merging this PR, I can certainly take care of that.
jwilsson: @shellscape No worries, I figured out a simple test for it.

And I'm glad the explanation made sense!

Patch:
diff --git a/lib/less-parser.js b/lib/less-parser.js
index 7b64943..db5550f 100755
--- a/lib/less-parser.js
+++ b/lib/less-parser.js
@@ -2,6 +2,7 @@ import Comment from 'postcss/lib/comment';
 import Import from './import';
 import Parser from 'postcss/lib/parser';
 import Rule from './rule';
+import Root from './root';
 import findExtendRule from './find-extend-rule';
 import isMixinToken from './is-mixin-token';
 import lessTokenizer from './less-tokenize';
@@ -10,6 +11,14 @@ const blockCommentEndPattern = /\*\/$/;
 
 export default class LessParser extends Parser {
 
+  constructor (input) {
+    super(input);
+
+    this.root = new Root();
+    this.current = this.root;
+    this.root.source = { input, start: { line: 1, column: 1 } };
+  }
+
   atrule (token) {
     if (token[1] === '@import') {
       this.import(token);
diff --git a/lib/root.js b/lib/root.js
new file mode 100644
index 0000000..9e73405
--- /dev/null
+++ b/lib/root.js
@@ -0,0 +1,14 @@
+import PostCssRoot from 'postcss/lib/root';
+import stringify from './less-stringify';
+
+export default class Root extends PostCssRoot {
+  toString (stringifier) {
+    if (!stringifier) {
+      stringifier = {
+        stringify
+      };
+    }
+
+    return super.toString(stringifier);
+  }
+}


Test patch:
diff --git a/test/postcss.spec.js b/test/postcss.spec.js
index ba7e48d..f4f15e3 100644
--- a/test/postcss.spec.js
+++ b/test/postcss.spec.js
@@ -62,4 +62,16 @@ describe('#postcss', () => {
         done();
       }).catch(done);
   });
+
+  it('should create its own Root node stringifier (#82)', (done) => {
+    const lessText = '@import "foo.less"';
+
+    postcss()
+      .process(lessText, { syntax: lessSyntax })
+      .then((result) => {
+        expect(result.root.toString()).to.eql(lessText);
+
+        done();
+      }).catch(done);
+  });
 });