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

fix: manipulation of node structures, proper stringifying #73

Merged
merged 3 commits into from Mar 7, 2019

Conversation

Projects
None yet
3 participants
@shellscape
Copy link
Owner

shellscape commented Mar 6, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

This PR fixes #69, wherein manipulating the child nodes for a particular node (in this case a Func node) did not reflect in stringifying of the node. This solution was derived from the following test code:

const { nodeToString, parse } = require('./lib');
const Punctuation = require('./lib/nodes/Punctuation');

const root = parse('rgb(100% 100% 100%)');

root.first.nodes.splice(1, 0, new Punctuation({ value: ',', parent: root.first }));
root.first.nodes.splice(3, 0, new Punctuation({ value: ',', parent: root.first }));

console.log(nodeToString(root));

The result of which is now:

rgb(100%, 100%, 100%)

Note that the parent property is being set in the code above, whereas the referenced issue did not set the property. This is required for PostCSS internals to accurately determine raws values to render.

Since we were previously only rendering the params property of functions (which is a string reference to unparsed function parameters), some additional work had to be performed to correctly capture and render spacing around and within the function parens. I believe this PR covers all of the various cases we may encounter.

@jonathantneal

This comment has been minimized.

Copy link
Collaborator

jonathantneal commented Mar 6, 2019

Reviewed the code and the snapshot. The parent requirement makes sense. Thanks!

@jonathantneal
Copy link
Collaborator

jonathantneal left a comment

I don’t see how params is handled here. Should there be a test to demonstrate that it is updated by nodes and/or vice versa?

@shellscape

This comment has been minimized.

Copy link
Owner Author

shellscape commented Mar 6, 2019

I don’t see how params is handled here

Are you speaking of the params property? If so, that property shouldn't be updated. It's a snapshot of what the function params were in the source. The documentation should be updated as well though, so that's a good catch.

@jonathantneal

This comment has been minimized.

Copy link
Collaborator

jonathantneal commented Mar 6, 2019

Sounds good to me, then. Thanks!

@jwilsson
Copy link
Collaborator

jwilsson left a comment

LGTM

shellscape added some commits Mar 7, 2019

@shellscape shellscape merged commit bc08be3 into master Mar 7, 2019

5 checks passed

ci/circleci: analysis Your tests passed on CircleCI!
Details
ci/circleci: dependency_cache Your tests passed on CircleCI!
Details
ci/circleci: node-v10-latest Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 93.17%)
Details
security/snyk - package.json (shellscape) No manifest changes detected

@shellscape shellscape deleted the fix/manipulation branch Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.