Patterns with Pattern Parameters Cannot Be Overwritten Downstream

[e2tha-e]

To replicate:

• Edit source/_patterns/04-pages/00-homepage.mustache
• Replace the content with this mustache code: {{> templates-homepage(emergency: true) }}
• This will render correctly in the Pattern Lab UI.
• Edit source/_patterns/03-templates/00-homepage.mustache
• Replace {{> molecules-alert }} with {{> molecules-alert(alertClass: 'error') }}
• This causes the emergency block to disappear instead of appearing with the correct alert class.

[bmuenzenmeyer]

To my knowledge, this is the expected behavior since pattern parameters were released - since encountering a pattern parameter causes immediate consumption of the data effectively closing off downstream changes. That said, I wouldn't call it perfect.

Does having a file like .source/_patterns/04-pages/00-homepage~emergency.json work for you?
Have you tried this in Pattern Lab PHP? I am going to install it fresh and try too.

[e2tha-e]

I remember doing recursive parameter submission all the time in Pattern Lab PHP, but wanted to be absolutely sure the latest version does it, and v1.0.0 definitely does it. Making the the exact code changes as in my first comment gets the alert class to show correctly.

Tilde-appended variants aren't the best option if you want to view components up and down the stream because you'll repeat definitions in multiple .json files, which goes against the DRY principle.

In any case, the emergency block shouldn't just disappear because of a downstream parameter submission.

[bmuenzenmeyer]

Hey @e2tha-e thanks for checking. I think I have a proposed solution that I might hack out and try as a branch. Welcome to suggestions for anyone too

[theorise]

I just wanted to say I came across this just yesterday and it is certainly something that would be great to have.

I may be abusing pattern parameters, but I am passing things such as data-attributes as JS hooks for components. In my use case:

We have a reusable icon atom, inside a reusable off-canvas organism. The icon acts as the close button:

{{> atoms-icon:icon--cross(iconDataAttributes:'data-off-canvas-toggle="toggle-target"') }

The molecule is included in certain templates, ideally as the following:

{{> organisms-off-canvas:off-canvas_small|off-canvas_right(offCanvasSelector:'toggle-target') }}

[bmuenzenmeyer]

Morning

The specifics of this issue seemed acceptable to me for a long time since only people using the more advanced features under certain scenarios would encounter this problem, and really one is left with many options as workarounds.

So I appreciate the additional information and 👍 - it's very valuable to me to see how people are using PL Node.

[e2tha-e]

@theorise That's exactly the sort of use-case which is not abuse, and would avoid the maintenance headache of making sure everything's up-to-date and correctly spelled in multiple places.

[bmuenzenmeyer]

Hey all - I pushed to the 250 branch a small tweak that I think fixes this - at least for the example that @e2tha-e originally reported. Could you all clang on it and see if it works?

Todo:

• confirm fix
• possibly improve performance
• revert mustache changes
• add a unit test covering this scenario
• rename pattern_hunter.find_parameters() - it's more than a getter

[vebersol]

Hi @bmuenzenmeyer,

I've been testing this branch in a running project our team is working on - based on PHP version. We're trying to migrate to node version, nevertheless this bug was a real problem and we're trying to help you to find a solution and I see this solution that came first is currently working in our real case.

So, I can confirm it's working in many different levels.

[bmuenzenmeyer]

@vebersol That is very good to hear as a first report of success. Thanks for letting me know! I am working on some unit test coverage right now too.

👍

[bmuenzenmeyer]

@e2tha-e could you please make sure this branch addresses your original issue?

[e2tha-e]

@bmuenzenmeyer This definitely does the recursion, but I also managed to get myself into an infinite loop while experimenting with this. Going to keep investigating.

[bmuenzenmeyer]

Uh-oh. Lemme know whatcha did if you reproduce, else I can poke at it more too

[e2tha-e]

@bmuenzenmeyer I looked at this more, and it's not an infinite loop. It's just a very long loop. What I'd been doing in Pattern Lab PHP is wrapping partial includes in conditional Mustache like

{{# include_foo }}
  {{> organisms-foo }}
{{/ include_foo }}
{{# include_bar }}
  {{> organisms-bar }}
{{/ include_bar }}

So a parameter submission like {{> templates-includer(include_foo: true) }} would only include organisms-foo and so forth. This is especially useful in Drupal-targeted projects because it would match Drupal's recursive template inclusion.

And to match a typical (big and complex) Drupal project, I'd do this a lot. However, since the recursion step you added to parameter_hunter.js doesn't check for the wrapping conditionals, it just recurses through every parameterized partial, which gets expensive exponentially.

I had actually hacked Pattern Lab Node's core in an earlier release to do the recursion with conditional checks, and it worked perfectly for the described scenario. However, I didn't have the time then to contribute back, and now, the core code has diverged to the point that I can't just drop in what I did and get it to work. If you don't mind, I'll keep plugging away at this until we get this feature to work in a performant manner.

[bmuenzenmeyer]

I understand. Thanks for the thorough explanation. We can keep this branch as is til you have time to get to it. If you find your time slipping away, let me know - otherwise, I'm excited for the performance improvement - hopefully the currently state can serve as a foundation to build from.

[vebersol]

Hi @bmuenzenmeyer @e2tha-e,

We are testing these improvements and currently the generation time for us is more 2 minutes and 50 seconds. In PHP the same amount of patterns takes 13 seconds. We have around 300 patterns.

I'm trying to figure out why the generation time is so different in both platforms.

It seems that PHP caches the generated patterns while the node version doesn't. Is that true or it might be an issue with recursion?

Thanks,
Vinícius Ebersol

[e2tha-e]

@vebersol If you are testing on the 250 branch, you're likely to get unreasonably long generation times with so many patterns. I have submitted a pr for a more streamlined approach, #277 . You're welcome to test out that code. Please do provide feedback on the generation time for that, as well any errors that may arise. As of right now, I think I've got the best possible code submission given my capacity to test, and don't expect to update the pr anytime soon, unless it's in response to feedback.

[vebersol]

Hi @e2tha-e, @bmuenzenmeyer,

Sorry about the delay to answer you, but it has been a long and busy time!

After a long time fighting against Patternlab PHP bugs and trying to migrate to Patternlab Node, without success, we've decided to create our own Atomic Design implementation. Way simple and faster than Patternlab. Obviusoly, Patternlab isn't the problem, but we realize we're using it for a different purpose but that one it was developed.

Basically, I did try to make it more Nodejs style (with all my limitation in this subject). After read Patternlab code for some time, I noticed that this Node version of Patternlab is so much PHP style, with all problems and all good solutions from PHP. We've discussed here in the agency and we've defined to move forward with our own implementation of Brad Frost's concept.

Patternlab was our main inspiration to build our Neutron engine - from scratch. I appreciate your support and I apologize not being able to help you to fix this issue in time.

[e2tha-e]

@bmuenzenmeyer @vebersol @geoffp I'm closing in on a pull request for this issue for Pattern Lab Node v2.x.x. I think I should have one submitted this week. The new performance is beating the work I did earlier this year by a large margin. While I've submitted several precursory prs, the main pr will probably be quite big. It would be difficult, if not impossible, to break it apart without breaking overall functionality.

[e2tha-e]

Also, I haven't tested my code with another templating engine. It sounds like the Handlebars engine might be stable enough to try testing with, so I'll eventually give that a go. Given how Mustache-centric parameter_hunter.js is, I am a bit confused as to how Pattern Lab Node will work with another templating engine.

[e2tha-e]

@vebersol At my last two jobs, my teams have also experienced your frustrations. There are probably hundreds of new software implementations inspired by Pattern Lab, including my own: Fepper

I've taken the fork and hack approach to solving this exact issue (#250) for Fepper. However, I'm also contributing this functionality back.

Regardless of whether we fork and hack, or start from scratch, I'm sure there's an enormous amount we end up learning. I've come to appreciate so many of the decisions made by the Pattern Lab PHP team, such as opting for the Mustache language, as opposed to something obviously more powerful, like Handlebars. By working through this issue, and actually trying to solve this problem with Handlebars, I found that a feature which might appear to make Handlebars more powerful, like its strict scoping of nested parameters, actually makes it unfit for the Pattern Lab use-case.

[tommcc]

Adding some examples here of cases where includes disappear due to combination of parameters or styleModifiers.

In the following scenarios, all of the buggy behavior is experienced when trying to view the organism's rendered output.

Scenario 1 - Atom disappears entirely

01-atoms/atom.mustache:

I should appear!

02-molecules/molecule.mustache:

{{> atoms-atom(b: true) }}

03-organisms/organism.mustache:

{{> molecules-molecule(a: true) }}

Scenario 2 - Atom disappears entirely

01-atoms/atom.mustache:

I should appear!

02-molecules/molecule.mustache:

{{> atoms-atom:x }}

03-organisms/organism.mustache:

{{> molecules-molecule(a: true) }}

[e2tha-e]

@tommcc I'd encourage you check out my Pattern Lab fork:
https://www.npmjs.com/package/fepper
https://github.com/electric-eloquence/fepper
Most recursive parameter submission issues have been resolved there. I'll test out your example myself later, but I have a feeling that your example will render correctly.

[bmuenzenmeyer]

I wonder if those two examples fail because in both, atom.mustache is not constructed to accept either styleModifiers or patternparameters, yet you supply them.

[tommcc]

@bmuenzenmeyer: Nope—the behavior is the same while utilizing all passed-in parameters. I just took them out to make the reproduction case even simpler.

[bmuenzenmeyer]

good to know, thanks

[e2tha-e]

@tommcc I can confirm your example works correctly in Fepper. 😃

[tommcc]

Thanks @e2tha-e for pointing Fepper. I like the features it brings, and will certainly keep an eye on it. I have heavily customized PL back- and front-ends, and for right now it makes more sense to stick with PL and hopefully help work through the small number of bugs that most affect us.

[bmuenzenmeyer]

I plan to move this use case into https://github.com/pattern-lab/starterkit-mustache-acidtest to understand it's spec compliance and further debug it myself.

[JustDoItSascha]

Is there any news on this issue? It's bothering me right now :(

[bmuenzenmeyer]

My comment above stands. If you want to open a PR into https://github.com/pattern-lab/starterkit-mustache-acidtest using the other issue there as guidance, that would help us understand it cross-platform and have a small, reproduceable test case to build from.

[JustDoItSascha]

It's exactly the test case which is part of the acidtest, I don't have to open a new PR. I'm just interesting if this issue will be fixed, because it's neraly a year old and I think it is a very important feature to create truly DRY patterns...

[bmuenzenmeyer]

Team priorities are currently geared toward #603 with smaller 2.X things per my semi-up to date task list: https://github.com/pattern-lab/patternlab-node/projects/1 You'll note fixing #539 is there.

If this issue is the same as #539 I see no reason to have both around.

[JustDoItSascha]

Too bad. It's a significant bug and I'm pretty sure it'd be easy for you to find it. Tried it myself, but the code is very complicated and without a guidance through the main parts of the code I have no chance to find the bug.

[bmuenzenmeyer]

this is now working against the OP use case as of https://github.com/pattern-lab/patternlab-node/releases/tag/v2.9.3