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

Using pattern parameters sometimes breaks includes #377

Closed
james-nash opened this issue Jul 1, 2016 · 5 comments
Closed

Using pattern parameters sometimes breaks includes #377

james-nash opened this issue Jul 1, 2016 · 5 comments
Assignees
Labels

Comments

@james-nash
Copy link
Contributor

I am using Pattern Lab Node v2.0.1 on Mac, with Node v6.2.1, using the Gulp Edition.

Expected Behavior

Including a pattern partial with parameters (as per http://patternlab.io/docs/pattern-parameters.html) should render the included pattern using the parameters supplied.

Actual Behavior

The included pattern is sometimes omitted completely.

It looks like under some circumstances the include works fine (and does so consistently) and under others it fails (also consistently). It's not yet clear what difference in circumstances causes this though.

Steps to Reproduce
  1. Create a pattern that includes another, using the pattern parameter syntax. E.g. {{> atoms-unordered( foo: 42 ) }}
  2. Rebuild your style guide
  3. Open the pattern
@bmuenzenmeyer bmuenzenmeyer self-assigned this Jul 2, 2016
@bmuenzenmeyer
Copy link
Member

I've identified what I believe to be the source of the error. (mostly for my notes)

in 1.3.0 pattern_assembler.js had this function to help find any partials.

  function findPartials(pattern) {
    var matches = pattern.template.match(/{{>([ ])?([\w\-\.\/~]+)(?:\:[A-Za-z0-9-_|]+)?(?:(| )\(.*)?([ ])?}}/g);
    return matches;
  }

This worked most of the time and found them regardless of whether or not they had a styleModifier or patternParameters.

The results of this function were used to then expand partials, within which we checked for patternParameters to process.

In 2.0.1, this logic was replaced by the patternengine-node-mustache regex starting here: https://github.com/pattern-lab/patternengine-node-mustache/blob/master/lib/util_mustache.js#L16 and called at https://github.com/pattern-lab/patternlab-node/blob/master/core/lib/pattern_assembler.js#L249 to accomplish the same task, but instead of finding a partial with a pattern parameterlike it used to, it now appears to fail.

I've confirmed this bug by using the atom you describe, and then toggling between

{{> atoms-unordered( foo: 42 ) }}

which the regex in question does not pick up, and

{{> atoms-unordered( foo: 42 ) }}
{{> atoms-unordered }}

which is picked up due to the presence of a simple partial alongside the parametered partial.

Tests need to be made to cover patternengine-node-mustache regexes, if they don't exist yet. Looking into that is the next step. I won't have time until late tonight.

cc @geoffp thought you might be interested.

@bmuenzenmeyer
Copy link
Member

I believe that pattern-lab/patternengine-node-mustache#1 should have fixed this issue. @james-nash please retest

@bmuenzenmeyer
Copy link
Member

Finally pushed this in a simpler fashion for you - if you are using an edition, running npm update should cover you.

@james-nash
Copy link
Contributor Author

Hi @bmuenzenmeyer, I just upgraded to 2.1.0 and can confirm that the bug is fixed for me.
I'm now successfully able to build my project which uses pattern params with non-string values.

Thank you so much for the quick turn-around! ...and apologies for the delayed response, I've been away for a few days and am only just catching up.

@bmuenzenmeyer
Copy link
Member

🍻 🎉

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

No branches or pull requests

2 participants