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

PatternEngines ongoing work, round 4 #208

Merged
merged 25 commits into from
Dec 14, 2015

Conversation

geoffp
Copy link
Contributor

@geoffp geoffp commented Dec 9, 2015

Here's the first cut of a working Handlebars plugin, plus a file-based unit test (@bmuenzenmeyer: doing it right?). It's just enough right now to prove that we can render a "hello world" atom and a "hello worlds" molecule that includes it twice as a partial.

Highlights:

  • @sghoweri's pattern_assembler fix is included
  • The Handlebars testing pattern tree tries to exactly emulate the structure of a real one, even though it's tiny
  • As in some of the nicer existing tests, the hbs tests set up a fake patternlab object and manually run process_pattern_iterative() and process_pattern_recursive().
  • These tests are replicating kind of a lot of buildPatterns() in patternlab.js. I wonder if there are some opportunites to refactor to make that process more testable, but I'm not sure exactly what they might be right now. I'm brain-fried for the day.

TODO:

  • Since I think we don't want to have every template engine ever as npm deps by default (right?), we probably shouldn't fail the build if, say, handlebars wasn't installed by the end user. I've included it in package.json for just during development.

bmuenzenmeyer and others added 20 commits November 26, 2015 15:11
Fix wrong class name for Error Input
…vated privileges

- THX: Thanks @RichardBray for the heads up
- ADD: Added a Prerequisites section to the README
real plugin files rather than a hard-coded list. Also, better organize
the PatternEngine setup functions to be cleaner, more functional and
more testable
added unit test for pseudopattern generation
fixed a bug where pseudopatterns were being added to the UI twice
explicitly sorted patterns before UI build

setting version to v0.16.0
more functional, which seems nice for unit tests; incorporate
@sghoweri's fix to the pattern_assembler
SHAME. "hello worlds" promoted to molecule!
Unit test to cover this

Resolves pattern-lab#171
Marking this version 1.0.0 - having achieved broad feature parity with PL PHP v1
if (config.debug) {
console.log('==================== FOUND NON-MUSTACHE FILE');
}
return;
return currentPattern;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super great idea. 💯

@bmuenzenmeyer
Copy link
Member

Here's the first cut of a working Handlebars plugin, plus a file-based unit test (@bmuenzenmeyer: doing it right?).

Right. As we've said before, the more unit tests we write this way vs total faking, the better. But I cringe when I force myself to do this and therefore am eager to spend more time making the whole product more unit testable.

@bmuenzenmeyer
Copy link
Member

These tests are replicating kind of a lot of buildPatterns() in patternlab.js. I wonder if there are some opportunites to refactor to make that process more testable, but I'm not sure exactly what they might be right now. I'm brain-fried for the day.

Agreed on both accounts

Since I think we don't want to have every template engine ever as npm deps by default (right?), we probably shouldn't fail the build if, say, handlebars wasn't installed by the end user. I've included it in package.json for just during development.

Right. A primary concern, handed down from Dave and Brad at the onset, was to keep Pattern Lab approachable. Now would could argue node, grunt, gulp etc are already skirting that line, but pattern engines to me are advanced functionality. That's a long-winded way of saying to the extent possible I'd like to us to ship with only mustache, and provide thorough documentation as to how to add another engine if someone wishes to use one. I haven't thought through what that means for our Travis CI builds; but my goal would be to always execute all tests.

@bmuenzenmeyer
Copy link
Member

@geoffp Let me know when you feel this is ready for another merge. I love how this is maturing. I did a quick scan of the commits, but have not run this yet.

@dmolsen
Copy link
Member

dmolsen commented Dec 9, 2015

My 2 cents re: pattern engines and defaults since my name was invoked...

I'm not sure if you can roll PL Node this way or if it would hinder testing but I'm trying to use "editions" as my way of organizing pattern engine-specific versions PL PHP. So the PL PHP default repo will use the Mustache pattern engine but we also have a PL PHP Twig repo. The standard for the repo names should give you the flexibility to add your own node editions if you like. eg. edition-node-handlebars-standard. At least for PL PHP this means that someone can download a zip of a pattern engine-specific version of PL and not have to worry about much. It's download and go.

That said, I think the reality is that we need to embrace package managers as it's the only way to get maintainable and customizable code/features. Not everything we do can be "core" and upgrades can't be copy & paste. From a PHP perspective this has meant rolling the bulk of PL into it's own package but hopefully it's worth it long-term. The projects just naturally have to get "a little more complicated." I definitely think that's ok.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 9, 2015

I'm glad you mentioned that, Dave, I don't think I'd realized that about the PL/PHP architecture. It does seem like providing pre-built "editions" for each engine type, with the template renderer's npm dependencies pre-installed, is a superior first-setup experience. But: does the PHP version support more than one engine per pattern tree? Can you have a "mixed" tree if you need it?

My team does have a need (at least we think we do) for a mixed pattern tree, at least in the short term. Of course, that requirement comes with some questions:

  • aren't all different kinds of partials registered in the same bucket right now? is that okay?
  • can an underscore pattern call a mustache partial, for example?

It seems like there are at least these three roads we could go down:

  1. Do homogeneous pattern trees (like PL/PHP?) that do only one engine type. I love the simplicity of this, but it may not address my team's needs.
  2. Let different template types coexist, but with the limitation that patterns can only address partials of the same template type. This seems very doable to me. We might have to split up partial registration per-engine, but we might not.
  3. Try to make different template types able to call each other. I know this sounds hard, and it may be untenable, but I have an inkling that Brian's way of processing style modifiers and pattern parameters, with the whole extendedTemplate way of pre-rendering things and passing them forward, could provide a way to do this!

I lean towards attempting the #2 solution as a default, and then at least try to assess the plausibility of #3.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 9, 2015

@bmuenzenmeyer got v1.0.0 merged!

@sghoweri
Copy link
Contributor

sghoweri commented Dec 9, 2015

@geoffp As things currently stand (at least with the Twig work I've been chipping away at), your 2nd solution is how things are setup out of the box. The way templating engines like Twig work out of the box, any partials getting referenced within that particular template must be of the same type. Everything remains separate by template type, however it all still gets compiled, registered and presented in the same interface within Patternlab.

IMO, the best approach might be to take a page out of the PL2 for PHP project(s) and begin to think about how we might be able to decouple and separate a few of the different moving parts that exist within Patternlab for Node. For example, the following could be split out into separate repos which can then be installed, updated, mixed and matched and customized individually via NPM.

  1. Core: Patternlab for Node JS logic that exists in the builder folder. This includes the core engine logic (builder/pattern_engines/pattern_engines.js) but not any template-specific engines (like Mustache) by default... however could be included in flavored "pre-built" versions of the repo for faster ramp up. Similar to the pre-packaged editions that @dmolsen mentioned.
  2. Engines: individually installed (and maintained) templating engines (Twig, mustache, handlebars, etc) which would be listed as a dependency to any pre-packaged demo patterns themselves. Could include Styleguide Kit Twig / Styleguide Kit Mustache, etc global patterns (in the source/_patternlab-files folder) as well or be separated out and installed independently.
  3. The PL "Viewer": aka, Styleguidekit Assets Default. The flavor of Patternlab's pattern / code viewer being used (in the public/styleguide folder)
  4. Separated Pattern Examples: example demo patterns that could be independently installed (ie. the patterns PL for Node currently ships with in the _patterns folder). Doing this means someone who wants to use handlebars templates doesn't have to npm install patternlab-node first, manually delete X folders, then add in their own.

FWIW, I've been working with PL2 for Twig for about 6+ months now on two huge projects and really do love the decoupled strategy Dave has been working on. I'd love to see what we can do to come up with a similar strategy for the Node version of Patternlab as well.

@dmolsen
Copy link
Member

dmolsen commented Dec 9, 2015

Building on @sghoweri's notes. I think adding package.json files to existing StyleguideKits and StarterKits is a possibility. Would take some work on install scripts and a certain alignment on the output of the two projects but, again, maybe it's possible.

Two historical notes on the set-up of PL2 PHP in case it helps you all. the components of a project. more details the on components.

@bmuenzenmeyer - time to revisit the spec?

@dmolsen
Copy link
Member

dmolsen commented Dec 9, 2015

@geoffp -

Clarification - based on the config one pattern engine is registered as the Renderer of Patterns at start-up. So then when I call something like $stringLoader->render() to render patterns in my pattern store it's only going to use the registered pattern engine.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 9, 2015

So, okay, this all makes sense. I'm super on board with @dmolsen's decoupled component architecture. There has to at least be a way to upgrade or swap out the moving parts without disturbing your actual library of patterns. Even though this complicates the architecture, I think Salem and Dave are right that NPM can make it pretty easy.

In the present-day monolithic world, my approach has been to include all supported engines, but only enable the ones whose renderer is installed by the end user. Even in that architecture, if we want to deliver easy-peasy starter kits like PL/Node+Handlebars or whatever, in the short term, it's just a matter of packaging and what's in the package.json by default.

But if pattern engines were factored out into their own npm packages, dependencies would come along for free. And no matter what starter kit you use, you can always just install another engine if you need a mixed tree. That seems cleaner.

As we were talking about splitting all these parts into npm modules, I started to get anxious about the awkwardness of git workflow when working on an npm module in-place, but I found npm link and now I think I feel better.

@dmolsen
Copy link
Member

dmolsen commented Dec 9, 2015

@geoffp -

npm link is interesting. In the Composer world I just have a development edition that loads dev branches of my projects as the packages so I can just commit or create new branches as needed. Initially I had assumed Composer was a rough copy of NPM but it doesn't appear to be so.

@bmuenzenmeyer
Copy link
Member

Wow this is a lot to digest in such short time!

I want to try to address this all in an organized, methodical fashion.

Modularization of Pattern Lab Node

A lot of ideas are coalescing around making Pattern Lab more modular, like PL PHP is doing. I definitely see npm playing a big role, and perhaps git submodules?, which I know little about.

@geoffp

There has to at least be a way to upgrade or swap out the moving parts without disturbing your actual library of patterns.
So, okay, this all makes sense. I'm super on board with @dmolsen's decoupled component architecture.

@dmolsen

I think adding package.json files to existing StyleguideKits and StarterKits is a possibility.

I definitely plan to work with you all on this. No need to have duplicate versions of these things if at all possible.

@sghoweri

IMO, the best approach might be to take a page out of the PL2 for PHP project(s) and begin to think about how we might be able to decouple and separate a few of the different moving parts that exist within Patternlab for Node.

This. This is the overall goal of this issue, which has been on the backlog for a while. From the history, you can see I put it off a number of times, and now the chickens come home to roost! 🐔 Every time I have to ask someone to copy/paste or alter a file manually during an upgrade, I feel like I've let them down. In some alternate reality I envisioned doing this before I embarked on pattern engines. I think not having a better separation of concerns naturally causes pattern engines discussions to include this topic, complicating both. More on how to unwind this knot in a bit...

Pattern Engines & Balance

@geoffp

But if pattern engines were factored out into their own npm packages, dependencies would come along for free. And no matter what starter kit you use, you can always just install another engine if you need a mixed tree. That seems cleaner.

I'd like to further pursue this mindset. I never imagined we'd be shipping with mustache, handlebars, and twig engines. That's doing a disservice to anyone installing. I think everyone agrees there is a better way.

The projects just naturally have to get "a little more complicated." I definitely think that's ok.

I've held very tight to the notion that Pattern Lab should be approachable. It was one of the tenants Brad spelled out early on with my involvement. I've been wrestling with the degree to which this is attainable, as tooling like grunt, gulp, BrowserSync, npm and such get injected more and more into our conversations. It's clear we are all advanced enough users, but what of the silent majority that just need it to work?

So the PL PHP default repo will use the Mustache pattern engine

I think that whatever shape the PL Node architecture takes, we should keep this directive in mind (until otherwise known).

Next Steps?

To me, a possible course of action looks like:

  1. Finish the (as Geoff puts it) monolithic version of pattern engines, with all engines inside the current structure. Include proper unit test coverage.
  2. Restructure Pattern Lab internals pursuant to this issue, bearing in mind a target alignment with repos that Dave has already restructured like styleguidekit-assets-default, starterkit-mustache-default, starterkit-mustache-bootstrap, starterkit-mustache-foundation. Alter these with package.json files as needed.
  3. Make sure all engines and functionality work with the inclusion of modules mentioned in 2.
  4. Start extracting pattern engines into other repositories.
    • Ship core with mustache? (my vote yes)
    • Create editions, as Dave mentioned?
      • I feel like we could maybe avoid this fragmentation by leaning on strong documentation and npm within the the core patternlab-node repository, but everyone's perspective is useful in formulating this. I've always hoped that people could go to one repository and get the fully skinny on what Pattern Lab Node is. Perhaps I'm too attached to this idea, and would like feedback from you all.

@bmuenzenmeyer - time to revisit the spec?
Perhaps this is a good topic for when we chat. Looking forward to it.

Cheers.

@dmolsen
Copy link
Member

dmolsen commented Dec 10, 2015

I've held very tight to the notion that Pattern Lab should be approachable. It was one of the tenants Brad spelled out early on with my involvement.

I think there is and has to be a continuum regarding approachability. Kind of like progressive enhancement. Someone will be able to install PL PHP from a pre-built zipped package or via Composer, PHP's package manager. Having a similar approach for your version of the project might be a way forward for you to balance managing complexity with Brad's original mission.

Regarding "editions"... this may be something that is more suited to PHP/Composer than Node/NPM so please don't feel tied to it. Composer has a nice create-project command that projects set-up like PL tend to use. I'm not sure NPM has a similar command.

@bradfrost
Copy link
Member

Heyo,
As resident stupid person, I figure I'd stick my head into the fire and weigh in.

@dmolsen mentioned "Brad's original mission", which might need a little clarification. A lot of my beef with a lot of tools and software out there is that the barrier of entry is way too high, dependencies are a never-ending rabbit hole, and documentation is full of language like "just do blah blah blah". So when we made Pattern Lab, there was a lot of effort put into keeping the requirements, overhead, and arrogance to a minimum. I'm thrilled Dave has done a great job of keeping that in mind over time, despite the software becoming more complex.

I'd very much love it if the new version of PHP Pattern Lab and the Node version continue to be tools people can spin up and get their heads around very quickly, yet still follow current best practices for real-world development. I'd be so bold as to say this is especially important to get right with the Node version. In my own Node journey, I find documentation for almost every tool & package to be chock full of arrogance, overhead, and assumptions. It would be so great to see Pattern Lab Node as a very approachable tool that doesn't require a master's degree to get off the ground. I'm not the one to suggest specific solutions on how to accomplish that, but I think that's a goal worth pursuing.

I feel like the whole ZIP concept Dave mentioned really achieves this balance between simplicity and robustness. A user can download a ZIP of a certain flavor of the project, run a command, and be up and running. Hooray! But if a sophisticated user is familiar with package managers, etc, they can opt to do the Real Deal environment set up that gives them a lot more power. I love how Dave called it a progressive enhancement of sorts.

Anyways, that's my two cents.

@bmuenzenmeyer
Copy link
Member

Thanks Brad - I think we can stay the course with focused effort, and would always appreciate any feedback on the Node documentation as it evolves, both here and on patternlab.io

@geoffp
Copy link
Contributor Author

geoffp commented Dec 10, 2015

On approachability, long/medium term:

So, we're experimenting with PL as a compositional tool for designers that have (or want) HTML+CSS skills. Off the top of my head, I can tell you that what's been not-so-approachable about that so far hasn't been PL internals or what's in the package.json, it's setting up the git + nodejs + npm + grunt toolchain on their laptops, and teaching them about those tools and the command line. It's a lot to take in for non-developers.

Feel free to say that those users aren't PL's main target audience -- I would certainly accept that -- but for those kinds of users, my feeling is that the experience wants to be as close to this as we can get:

  1. Download a .zip and unzip
  2. Double-click a thing
  3. See PL in your browser and start editing HTML+CSS.

On next steps:

@bmuenzenmeyer I like your proposed roadmap.

@bmuenzenmeyer
Copy link
Member

Off the top of my head, I can tell you that what's been not-so-approachable about that so far hasn't been PL internals or what's in the package.json, it's setting up the git + nodejs + npm + grunt toolchain on their laptops, and teaching them about those tools and the command line. It's a lot to take in for non-developers.

The target audience has always been as wide as possible. The Node version is a bit more inherently advanced-entry... since it leans so heavily on these task runners. PL PHP is more approachable IMO, with its shipped command files (I think Mac only...)

But that doesn't mean we can't do better!

  1. Download a .zip and unzip
  2. Double-click a thing
  3. See PL in your browser and start editing HTML+CSS.

This issue exists to handle this better. I agree it'd be nice - it's just never been prioritized high against working on other things. Probably wouldn't take too long to create a couple different files users could click on to start. Getting this on the roadmap is important. I'll put some meat on its bones soon and tag it up for grabs if someone wants to tackle it.

Also: updated the roadmap to somewhat reflect all this discussion. Will continue to grow it out as needed.

@dmolsen
Copy link
Member

dmolsen commented Dec 10, 2015

@geoffp & @bmuenzenmeyer -

It kills me that Apple no longer supports the .command files. I understand why they don't but they definitely did make things as easy as "double-click."

@geoffp
Copy link
Contributor Author

geoffp commented Dec 10, 2015

@dmolsen Maybe it would work to create a super thin AppleScript for Mac, PowerShell for Windows, and .desktop file for Linux that do nothing but launch the "real" shell scripts.

@bradfrost
Copy link
Member

Yep

@geoffp
Copy link
Contributor Author

geoffp commented Dec 14, 2015

Hey guys, sorry for the lull! I had the simplest problem holding me up: I was trying to render our library of Handlebars templates, and the log file, patternlab.json, grew to 180MB and then barfed. Turns out that the Handlebars engine objects in oPatterns were sneaking circular references in there.

The solution I landed on was to pass a custom replacer function to JSON.stringify() that doesn't try to stringify the oPattern.engine object.

There's also a new optional engine method, registerPartial(), which is there to handle pattern registration for engines that have their own internal partial registration system; Handlebars happens to be one. It's called by addPattern() in pattern_assembler. This keeps pattern registration nice and separate from pattern rendering.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 14, 2015

And hey, @bmuenzenmeyer, thanks for the shout-out on the MS Dev Show! That was fun to hear. It got passed around at Target.

@bmuenzenmeyer
Copy link
Member

@geoffp you bet on the shoutout - It's eternally cool that you guys even deem it worthwhile, and are actively investing in it. It's means the world to me, really. When the dust settles a bit I'd love to learn more about what you guys are building - or if you could share some screen shots with me via DM or email, would be great!

Sorry bout patternlab.json - it's been growing bigger and bigger over time, and was WAY more manageable and useful during early dev. Still, nice to see raw state sometimes. I've been meaning to try out something like Webstorm that actually lets me debug a bit easier.

Per some conversations with Dave - I am hoping you and Salem can get engines relatively stable, and then I'll likely embark on a solitary crusade to restructure. I think it might be easier if one person does it. I'll likely need help/info from you two along the way.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 14, 2015

Yeah, I've had some pretty bad times working with more than one person while moving lots of files and dirs around. Or even fixing widespread indentation issues. The merges are horrible unless you flip just the right switches.

We should do a Google Hangout or whatever sometime soon, if you're game (and if that's permitted on our network) and we'll show you what we do! I'm sure anyone in this thread who's interested would be welcome to join.

I think this is ready for merge now. @sghoweri, how's your branch? I'd love to get that work in here.

@bmuenzenmeyer
Copy link
Member

@geoffp can you test out adding pattern-engines to the branches list inside https://github.com/pattern-lab/patternlab-node/blob/master/.travis.yml

I want to see if it will kick off Travis builds

bmuenzenmeyer pushed a commit that referenced this pull request Dec 14, 2015
PatternEngines ongoing work, round 4
@bmuenzenmeyer bmuenzenmeyer merged commit 4f52169 into pattern-lab:pattern-engines Dec 14, 2015
geoffp pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
geoffp pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
geoffp pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
bmuenzenmeyer pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
geoffp pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
geoffp pushed a commit that referenced this pull request Feb 23, 2018
PatternEngines ongoing work, round 4
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.

None yet

7 participants