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.

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

Supply a Root stringifier #82

Merged
merged 2 commits into from
Jul 2, 2017
Merged

Supply a Root stringifier #82

merged 2 commits into from
Jul 2, 2017

Conversation

jwilsson
Copy link
Collaborator

@jwilsson jwilsson commented Jul 1, 2017

Which issue # if any, does this resolve?
No issue, see #81 for some related discussion.

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
  • 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
Copy link
Owner

@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
Copy link
Collaborator Author

jwilsson commented Jul 1, 2017

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

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({ // This is where it'll currently fail
        word: 'foo',
    }));
}).catch((error) => {
    console.error(error);
});

I hope this makes sense!

@jwilsson
Copy link
Collaborator Author

jwilsson commented Jul 1, 2017

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:

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

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
Copy link
Owner

mind blown

Amazing detective work, sir. The explanation and fix make total sense. Thanks for all of that.

@shellscape
Copy link
Owner

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
Copy link
Collaborator Author

jwilsson commented Jul 2, 2017

@shellscape No worries, I figured out a simple test for it.

And I'm glad the explanation made sense!

@shellscape shellscape merged commit 4caa2d3 into shellscape:master Jul 2, 2017
@jwilsson
Copy link
Collaborator Author

jwilsson commented Jul 6, 2017

@shellscape Is there anything I can help you with to get a new version up on npm?

@shellscape
Copy link
Owner

1.1.0 is out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants