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

Configurable paths galore #216

Merged
merged 6 commits into from
Jan 1, 2016

Conversation

geoffp
Copy link
Contributor

@geoffp geoffp commented Dec 23, 2015

@bmuenzenmeyer, check this out.

This level of configurability is what was needed for me to get it to build our pattern library from inside an npm module, which we have mostly working! We actually did pretty much this when we forked a year ago and (unfortunately, in hindsight) started moving files all over the place. It's nice to see the same pattern re-emerge.

NOTES:

  • the use of path.resolve() is debatable. I could have stuck with manual concatenation, or used path.join(), which doesn't convert to absolute paths, but it felt safer to use it than not to, so I kept at it. It seems to work, and it does free us from having to do the jsonFilename.substring(2) thing in the pattern assembler.
  • doing a completely separate path for each entry in config.json is also debatable, but it seems more useful to me to err on the side of maximum flexibility. It's not that repetitive.
  • I've wired up the gulpfile to use these paths during the build step, but not yet the gruntfile.

to use path.resolve and/or path.join rather than manual string
concatenation to manipulate paths.

Conflicts:
	builder/patternlab.js
	builder/pseudopattern_hunter.js
	test/engine_handlebars_tests.js
	test/pattern_assembler_tests.js
@bmuenzenmeyer
Copy link
Member

Hey @geoffp I made a quick pass at this on mobile last night, and will try to review more thoroughly during the long weekend.

Thanks for this!

@bmuenzenmeyer
Copy link
Member

Hey @geoffp just wanted to reach out as I didn't get to this yet. I am almost through fuzzy pattern support, and tracked down a nasty regression tonight... so reviewing this fell to the wayside.

That said, this PR is going to be a BIG STEP IN THE RIGHT DIRECTION. Thanks!
Stay tuned.

@geoffp
Copy link
Contributor Author

geoffp commented Dec 29, 2015

I hope so! I figured that whatever winds up happening with modularization and directory structure changes, we're going to have to be able to move stuff around.

Fuzzy patterns sound interesting, what are those?

@bmuenzenmeyer
Copy link
Member

Basically it's a shorthand syntax that does a substring match of the pattern key's name. It's a gap with official PHP v1 that didn't make it in to PL NODE v.1.0.0

Taken from:

The shorthand syntax also allows for fuzzy matching on pattern names. This means that, if you feel your pattern name is going to be unique within a given pattern type, you can supply just the unique part of the pattern name and the partial will be included correctly. For example, using the shorthand syntax this partial could be written as:

{{> atoms-16x9 }}

Important: If you include 16x9 in another pattern the PHP version of Pattern Lab may find that first depending on how your patterns are organized.

I am tracking progress in #202 and a feature branch. I am hoping it won't conflict too much with engine work (more unit tests!), but the main difference is outlined in 980118f pattern_assembler.getpatternbykey

@geoffp
Copy link
Contributor Author

geoffp commented Dec 29, 2015

Cool! At a glance, that patch looks like it won't conflict much with the engines branch.

@bmuenzenmeyer bmuenzenmeyer modified the milestone: v1.1.0 Dec 30, 2015
@bmuenzenmeyer
Copy link
Member

Fuzzy patterns pushed to dev. working on this next (sometime this week)

@bmuenzenmeyer
Copy link
Member

First pass, this ran on gulp no sweat. I am going to accept this after I do a bit more testing outside of the main filesystem, as you've mentioned to me earlier. From there, I will create one final issue with remaining to-do items, like grunt support and anything else that pops up.

I will want to include a blog post with this as well.

@bmuenzenmeyer
Copy link
Member

  • the use of path.resolve() is debatable. I could have stuck with manual concatenation, or used path.join(), which doesn't convert to absolute paths, but it felt safer to use it than not to, so I kept at it. It seems to work, and it does free us from having to do the jsonFilename.substring(2) thing in the pattern assembler.

I find your approach better

  • doing a completely separate path for each entry in config.json is also debatable, but it seems more useful to me to err on the side of maximum flexibility. It's not that repetitive.

I first found this questionable, but agree it affords the most flexibility

  • I've wired up the gulpfile to use these paths during the build step, but not yet the gruntfile.

I can do grunt after merge, which is now. Happy New Year!

bmuenzenmeyer pushed a commit that referenced this pull request Jan 1, 2016
@bmuenzenmeyer bmuenzenmeyer merged commit d01383a into pattern-lab:dev Jan 1, 2016
@bmuenzenmeyer
Copy link
Member

Moved any remaining cheklist items to #220

@bmuenzenmeyer
Copy link
Member

@geoffp Would love to know what mechanism you are using, on say, an npm update of patternlab, to repopulate your config.json entries? Just manual for now or something upstream merging the json map?

@geoffp
Copy link
Contributor Author

geoffp commented Jan 4, 2016

Happy new year!

I just added a task to what I now think of as the "user" gulpfile (which I manually copied, once, from the npm module to the project root) that sets PL's default config aside and then injects the "user" config.json (Docker-style) into the npm module. It does this each time gulp is run.

I now realize that this might not work for an upgrade because I assumed that npm would delete the whole node_modules/patternlab-node directory and rebuild it on upgrade. If it just updates files in place, I'll have to tweak it a little. I should really test that.

The gulp task is this:

// merge/copy config
gulp.task('userconfig', function (done) {
  function copyConfig () {
    // copy the user config from the root into the place of the config that
    // ships with PL
    console.log('Backing up original config file.');
    return gulp
      .src(userConfigPath)
      .pipe(gulp.dest(upstreamPath))
      .on('end', function () {
        console.log('Done messing with config files.');
        done();
      });
  }

  // if we haven't already backed up the stock config, copy the original config
  // to config.json.orig
  fs.access(stockConfigPath, function (err) {
    if (err && err.code === 'ENOENT') {
      fs.rename(effectiveConfigPath, stockConfigPath, copyConfig);
    } else {
      console.log('Config file backup already exists.');
      done();
    }
  });
});

gulp.task('restorestockconfig', function (done) {
  // restore the stock config: copy config.json.orig back onto config.json
  fs.rename(stockConfigPath, effectiveConfigPath, done);
});

Also, I made this little function:

function paths () {
  return require('./config.json').paths;
}

That brings in the paths from the config file whenever called; I then added the userconfig task as a dependency to any tasks that need to read paths, to be sure that the config is already in place. So the clean task looks like this:

//clean patterns dir
gulp.task('clean', ['userconfig'], function(done) {
  del.sync(
    [
      path.resolve(paths().public.patterns, '*'),
      path.resolve(paths().public.styleguide, '*')
    ],
    {force: true}
  );
  done();
});

I'll try an npm update and see what happens.

EDITS: I can't write this early in the morning.

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.

2 participants