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

Downstream projects can now require grunt and gulp tasks, no need to copy them anymore. #245

Closed
wants to merge 2 commits into from

Conversation

tbouron
Copy link

@tbouron tbouron commented Feb 11, 2016

Supersedes #244.

This is a follow up of #221.

Here is how a downstream project can use patternlab:

  • Intall patternlab as dependency: npm install patternlab-node --save-dev
  • Copy the config.json file: cp node_modules/patternlab-node/config.json patternlab-config.json Here is an example that uses patternlab files for the styleguide but my project's patterns, data, image, etc:
{
  "paths" : {
    "source" : {
      "root": "./source/",
      "patterns" : "./source/_patterns/",
      "data" : "./source/_data/",
      "styleguide" : "./node_modules/patternlab-node/core/styleguide/",
      "patternlabFiles" : "./node_modules/patternlab-node/source/_patternlab-files/",
      "js" : "./source/js",
      "images" : "./source/images",
      "fonts" : "./source/fonts",
      "css" : "./source/css/"
    },
    "public" : {
      "root" : "./public/",
      "patterns" : "./public/patterns/",
      "data" : "./public/data/",
      "styleguide" : "./public/styleguide/",
      "js" : "./public/js",
      "images" : "./public/images",
      "fonts" : "./public/fonts",
      "css" : "./public/css"
    }
  },
  ...
}
  • If your are using gulp, here is the content of the gulpfile.js:
var gulp = require('gulp'),
    config = require('./config.json');

// Load gulp-build tasks to the current gulp and inject the local configuration
require('./builder/gulp-build')(gulp, config);

// Your gulp tasks go here
...

gulp.task('default', ['pl:lab']);
  • If your are using grunt, here is the content of the Gruntfile.js:
module.exports = function(grunt) {

    var config = require('./config.json');

    grunt.initConfig({
        // Your grunt configuration goes here
        ...
    });

    // Load grunt tasks to the current grunt and inject the local configuration
    require('./builder/grunt-build')(grunt, config);

    // Your grunt tasks go here
    ...

    grunt.registerTask('default', ['patternlab', 'copy:pl_main', 'copy:pl_styleguide']);
};

@tbouron tbouron changed the title Require grunt gulp Downstream projects can now require grunt and gulp tasks, no need to copy them anymore. Feb 11, 2016
@tbouron
Copy link
Author

tbouron commented Feb 18, 2016

Hi @bmuenzenmeyer, @EvanLovely. Any news on the review? Just to let you know, I'm using my branch as a base for my downstream project, works great :)

FYI, I rebased on the latest dev branch so the PR is mergeable again.

@bmuenzenmeyer
Copy link
Member

Hi @tbouron

I haven't forgotten about this, and I certainly don't believe in the veto of a PR by ignoring it.

I like the idea of namespaced tasks and generally like the approach - but did want the dust to settle on 1.1.0 a bit and have others weigh in if they fancied. 😄

We are not heavily utilizing PL Node at work in a downstream scenario - but I know @geoffp is - so I'd appreciate feedback from him too.

Some concrete feedback: As we discussed in the old PR I still believe that both package.json files should be present. Just because Pattern Lab Node can be used as a true dependency, I am not ready to drop support for it as a standalone repo just yet. I think there are plenty of people that wish it could be done both ways. Perhaps I need some convincing, or evolution of my thoughts on the matter, as the dependency route is no doubt more maintainable (albeit a bit more setup). 😄

@geoffp
Copy link
Contributor

geoffp commented Feb 18, 2016

I'm checking this out now.

@tbouron
Copy link
Author

tbouron commented Feb 18, 2016

Hey @bmuenzenmeyer, thanks for your reply and good to know how things work around here. Every project has its own way of managing things so now I know :)

Regarding the package.json, I probably failed to explain to you, let me try again. Having only one package.json does not mean it drops the standalone support, on the contrary. All gulp and grunt dependencies are installed together, regardless of which toolchain you want to use. Which means that if you want to use grunt, you can. Same applies for gulp. You can even play and switch between both tools.

Does it make more sense? :)

@geoffp
Copy link
Contributor

geoffp commented Feb 18, 2016

@bmuenzenmeyer, this appears not to eliminate the possibility of running standalone, but I do need to do gulp pl:serve rather than gulp serve.

My gut feeling is that a build process is something that people will inevitably want to customize. I found it hard to customize the previously included gulp tasks because their task dependencies can't be changed, for example. In one case, that was a deal-breaker, so I just copied the tasks and changed them.

If we're eventually going to get to a scenario where the thing people git clone is a "hello world" starter project that includes the main patternlab-node as an NPM dependency, then that project would contain a fully formed build process and you wouldn't have to copy anything by hand. And it would be infinitely customizable -- though the downside would be that you wouldn't (automatically) get upstream changes to the gulp & grunt stuff.

So I guess one value judgement at play here is, what's more valuable, a very DRY, off-the-shelf build process that works for you until it doesn't, or an easy-to-clone-and-customize build process? I find it hard to say, even for myself.

Maybe there's a way of sharing build code that isn't as heavy-handed as specifying whole tasks on behalf of the user. I know gulp people often write about splitting the handler functions off of the tasks, and sharing those. Thoughts?

@bmuenzenmeyer
Copy link
Member

Responding to @tbouron

Having only one package.json does not mean it drops the standalone support, on the contrary. All gulp and grunt dependencies are installed together,

This is what I take issue with, specifically. If I understand correctly, a user is going to be installing dependencies they will wholly never use. My solution to that was to have them pick one of the two package.json files we currently ship. I suppose this conundrum is the cost of supporting both toolchains...

@tbouron
Copy link
Author

tbouron commented Feb 18, 2016

This is what I take issue with, specifically. If I understand correctly, a user is going to be installing dependencies they will wholly never use. My solution to that was to have them pick one of the two package.json files we currently ship. I suppose this conundrum is the cost of supporting both toolchains...

I understand your stand but is it really an issue? Having more dependencies installed does not impact on the final product, except for the initial install time, but not by a large gap. We are talking about 8 grunt/gulp plugins which will increase the elapsed time by nearly 0 to 6 seconds tops, depending of your broadband. Also, having 2 distinct package.json won't work for a full NPM dependency point of view which was the primary reason why I went down that road. I reckon installing few more "unused" dependencies is an acceptable compromise.

Now, I do understand that you see it differently. In that case, I would keep using my fork and bringing upstream changes manually - the beauty of open source :)

@bmuenzenmeyer
Copy link
Member

Now, I do understand that you see it differently. In that case, I would keep using my fork and bringing upstream changes manually - the beauty of open source :)

I appreciate the civil discourse, thanks!

I also appreciate the reasoned thoughts. I'm coming around.

I think it might be time.

I propose:

patternlab-node core, builder, tests instructions on how to install what, it being the mothership
patternlab-grunt patternlab-node as dependency, grunt harness, (someday) pointer to same startup mustache patterns and styleguide repo as patternlab-php
patternlab-gulp patternlab-node as dependency, gulp harness, (someday) pointer to same startup mustache patterns and styleguide repo as patternlab-php

In a way - these test/reference repos are almost there:

Thoughts?

@tbouron
Copy link
Author

tbouron commented Feb 19, 2016

@bmuenzenmeyer That would work for me :)

I actually never paid attention to what patternlab-php did but it's quite good and I really like their starter kits that use different harnesses to frameworks like bootstrap, twig, etc.

@geoffp
Copy link
Contributor

geoffp commented Feb 19, 2016

@bmuenzenmeyer
Copy link
Member

Just a communication update: I'm not ready to split the project into a couple repositories just yet, so please don't fret if this sits for a bit.

@EvanLovely
Copy link
Member

You wouldn't have to split the repo: just make a sub-folder that has it's own package.json in it. You can even register multiple npm packages from the same repo if you want! Take a look at how Babel does it.

@bmuenzenmeyer
Copy link
Member

woah nelly TIL that was a repo strategy! Thanks for sharing. I haven't committed to any one path yet but I do tend to like the path Dave has paved to an extent.

@geoffp
Copy link
Contributor

geoffp commented Feb 26, 2016

That is crazy cool -- I don't know if we want to do that, but damn is it nice to have an NPM Jedi hanging out.

Thinking about dependencies in the pattern-engines world is a challenge: the plan had been for only mustache to be installed by default, and for the user to npm install whatever renderers they need (handlebars, underscore, whatever), but if PL is a module dependency, I don't know if we can tell it to pull in dependencies that aren't in its package.json by default. We may have to split off the pattern-engines into their own modules, too. Maybe the one-repo-many-packages model works well for housing the pattern engines...

Now that I think about it, it does kind of feel right for the "starter kits" that include PL as a module to be the ones to list pattern-engine dependencies. After all, the user knows what kind of templates they'll need, not us.

Thinking out loud here.

@EvanLovely
Copy link
Member

Welcome! It'd be interesting to just integrate consolidate.js and then people could use any of the 30-odd templating engines that it supports. I work at a Drupal shop & we're very interested in getting Twig in as a templating language.

@geoffp
Copy link
Contributor

geoffp commented Feb 26, 2016

@sghoweri, are you still with us? We have some more interest in a twig engine. Would you mind trying another PR against the pattern-engines branch?

@sghoweri
Copy link
Contributor

@geoffp Sorry for being AWOL, in the process of transitioning over to an awesome new job opportunity.

I can definitely help with this - had a couple breakthroughs in the last couple weeks with getting the Twig.js engine to work with the shorthand include / embed / extend syntax PL uses, in addition to getting pseduo pattern data to get used in the context of an include as well.

I know previously I've had issues with passing in / having access to the Twig template's absolute path data (when the object_factory's this.engine.renderPattern(this.extendedTemplate, data || this.jsonFileData, partials) function fires) since that particular piece of data isn't accessible via the data parameter here (but is available if we pass in this instead).

Probably need another PR. I'll see what I can do.

@geoffp
Copy link
Contributor

geoffp commented Feb 26, 2016

@sghoweri, that's fantastic, and congrats on the new job! Keep us posted!

Do you mean the syntax extensions like style modifiers and pattern parameters? I'm completely copping out on those for the Handlebars engine since Handlebars pretty much addresses the use cases for those in its core feature set.

@sghoweri
Copy link
Contributor

@geoffp Correct. Twig provides at least some help with managing data inheritance / merging (and the rest I've had pretty good luck getting to work via gulp-data), however I had to add in a bit of custom code in order to get (for example), {% include "atoms-buttonprimary" %} to resolve to the "atoms-button" twig partial while still using the data from "atoms-buttonprimary.json.

From what I can recall, this wasn't previously possible, at least with the Patternlab 2 for Twig edition.

Spending some time working on this today -- will keep you posted!

@geoffp
Copy link
Contributor

geoffp commented Feb 29, 2016

@sghoweri Excellent! Gogogo!

Also, heads up: we just did a big white space cleanup in both the main line and the pattern-engines branch, so it would probably be a good idea to get up to date with the latest pattern-engines and get the big nasty whitespace-heavy merge out of the way sooner rather than later.

@sghoweri
Copy link
Contributor

@geoffp Sweet - thanks for the heads up!

I'll save the details till I get this PR wrapped up (tonight or tomorrow) but I'm making progress getting Twig to cooperate with the latest patternlab engines setup. Very close to at least getting the basics at least (native Twig includes / embeds and extends with PL lineage matching) -- should have something up tonight or tomorrow.

Probably makes the most sense to get the MVP out first before I take a crack at implementing the psuedo-pattern include piece.

@bmuenzenmeyer
Copy link
Member

While I appreciate all the work and discussion put into this PR more than you know, it has diverged significantly from what the repo was like in winter. I think that many parts of this influenced how I structured edition-node-gulp's gulpfile, but please look there for any improvements.

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

Successfully merging this pull request may close these issues.

5 participants