Skip to content
This repository has been archived by the owner on May 1, 2020. It is now read-only.

Proposal to fix an underscore-engine issue: configurable output file suffixes #26

Closed
geoffp opened this issue Jul 20, 2016 · 20 comments
Closed

Comments

@geoffp
Copy link

geoffp commented Jul 20, 2016

As the maintainer of a design system that uses Underscore templates, I use Pattern Lab / Node to maintain it, and have written a pattern engine to be able to author Underscore templates. Unfortunately, our library of Underscore templates consists of files named with the .html extension. This destroys the frontend viewer's ability to render them, since the raw template overwrites the rendered output during the build step; they have exactly the same filename.


Therefore, as per @dmolsen's suggestion, I propose that the naming of these output files be configurable. I propose, for simplicity, that this be done with the addition of configurable suffixes for these output files, which would appear at the end of the base file name but before the file extension. Here is a proposal for the configuration structure, with the present-day scheme as defaults:

{
  ...
  outputFileSuffixes: {
    rendered: '',
    rawTemplate: '',
    markupOnly: '.markup-only',
  }
  ...
}

The timeline for this feature is ASAP. This is blocking our migration to Pattern Lab 2.0.

If I had permissions to do so, I would tag this needs-implementation because no POC implementation yet exists.

/cc @pattern-lab/voting-members

@dmolsen
Copy link
Member

dmolsen commented Jul 20, 2016

@geoffp -

In general I'm fine with this. I think there's only one issue at the moment. The name of the tab in the front-end is derived from config.patternExtension. Edit: the copy on the pattern tab will be derived from the extension of the file. Is that ok in your case or do we need an override for that too? Seems like in your case you'd have an HTML tab for markup only and HTML tab for the raw template.

System Input

The following options defined in the config:

outputFileSuffixes:
 - rendered: ''
 - rawTemplate: ''
 - markupOnly: '.markup-only'

System Output

Files will be rendered with the appropriate suffixes. .html will be automatically added to rendered and markupOnly. The original file extension will be added to rawTemplate.

@geoffp: So in this scenario I assume the patternExtension is html?

outputFileSuffixes will be passed as part of the existing config object.

Behavior

If these options are not configured and therefore undefined then the front-end will default to:

  • for config.outputFileSuffixes.rawTemplate: ''
  • for config.outputFileSuffixes.markupOnly: '.markup-only'

.html will be automatically added to the markupOnly option. config.patternExtension The original file extension for the pattern will be added to rawTemplate option.

Building pattern paths should set the "rendered" option so no need to be explicit on the front-end about it.

Example

A config with:

patternExtension: 'html'
outputFileSuffixes:
 - rendered: '.rendered'
 - rawTemplate: ''
 - markupOnly: '.markup-only'

Will end up with files like:

long-filename-here.rendered.html
long-filename-here.html
long-filename-here.markup-only.html

And two tabs called HTML.

If this is ok I can vote with a thumbs up.

@bmuenzenmeyer
Copy link
Member

I am in favor of the proposal - making those filenames configurable makes sense.

@dmolsen -

In general I'm fine with this. I think there's only one issue at the moment. The name of the tab in the front-end is derived from config.patternExtension. Is that ok in your case or do we need an override for that too?

This issue has come up on two occasions now:

  • Mixed pattern trees, where, for example, handlebars and mustache patterns might coexist
  • Handlebars engine use where there was conflict between a user's .handlebars files and the configured .hbs setting.

Does this setting only control the tab label? If so, could it be derived from the pattern's file itself?

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

@bmuenzenmeyer -

Argh. Considering patternExtension is passed as part of the patternData (at least for PHP) then, with some modifications to how panels are gathered on the front-end, I think it would be doable. That tab could reflect the actual extension of the original pattern itself as opposed to the PatternEngine. It will in no way address the question I had for @geoffp but it's doable. I'll edit my round-up as appropriate to reflect that change.

To be clear, PL/PHP doesn't support a mixed "pattern tree." I don't really intend on going that route either. If your PatternEngine set-up allows it cool. Looking through your code it seems it does. My rules are a bit stricter than that and I only invoke one PatternEngine.

When @geoffp chimes in on what I wrote up I'll vote.

@geoffp
Copy link
Author

geoffp commented Jul 21, 2016

@dmolsen -

I do think we should be able to change the title of the template tab based on each pattern's file extension, but in the Node case, the pattern engine loader is able to produce each engine's human-readable name (indirectly, via each Pattern object) based on the file extension, so I think in our case we'd prefer to use that value if that doesn't mess anything up for the PHP version. That way we wouldn't see HTML/HTML in the tabs, we'd see Underscore/HTML.

Apart from that clarification, I agree with everything you posted!

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

@geoffp -

Ah, got it now.

👍 from me on this change and I'll move it up the queue.

I have some fixes that need to happen to styleguidekit-assets-default anyway. This should be able to be rolled out in the short-term without changes to either core. Once it's been implemented in core things should all just work.

@bmuenzenmeyer
Copy link
Member

👍

cheers!

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

My "panels" implementation/configuration needs to be revisited anyway. I'll put another proposal in for that. This works in the short-term.

@bmuenzenmeyer
Copy link
Member

Thanks Dave - lemme know if ever you need help with the front end, I should really become more familiar with it anyways.

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

Sorry, have to re-open this. The edited description of the solution based on feedback from @bmuenzenmeyer doesn't jive with what @geoffp said.

@geoffp -

So the underscore patterns are ending with .underscore but the raw output needs to be written as .html instead? So the raw output for foo.underscore goes to foo.html? And the rendered version is foo.rendered.html? If so what I proposed isn't actually going to address your problem. I can revisit this afternoon and get a solid example going. Just double-checking. Sorry.

@dmolsen dmolsen reopened this Jul 21, 2016
@geoffp
Copy link
Author

geoffp commented Jul 21, 2016

@dmolsen - no, I think we had it right before. Our raw underscore templates end in .html, that's why they're overwriting the rendered version in the current scheme.

And -- thanks for the quick analysis and triage!

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

I'm probably just overthinking this... Please correct me if I'm wrong.

So the original pattern that uses underscore would be something like:

 source/_patterns/atoms/general/brand-colors.underscore

Using the following rules:

outputFileSuffixes:
 - rendered: '.rendered'
 - rawTemplate: ''
 - markupOnly: '.markup-only'

and the fact that the file extension on the pattern is underscore this would output:

 long-filename-here.rendered.html
 long-filename-here.underscore
 long-filename-here.markup-only.html

This is because the edited version of my proposal said "The original file extension for the pattern will be added to rawTemplate option." We're sort of back to square one.

So if we make the suffixes explicit to include the final file extension they would look like:

outputFileSuffixes:
 - rendered: '.rendered.html'
 - rawTemplate: '.html'
 - markupOnly: '.markup-only.html'

Then this would be the explicit output regardless of original pattern extension:

 long-filename-here.rendered.html
 long-filename-here.html
 long-filename-here.markup-only.html

The original pattern extension could still be used for the tab. The original behavior I outlined above should still work appropriately. For PHP I'll use the existing behavior as a default in case those options are missing (e.g. rawTemplate will be the pattern extension).

Let me know how this might affect node. Still doable. I just want to get it right.

@geoffp
Copy link
Author

geoffp commented Jul 21, 2016

Not quite.

The original pattern that uses underscore has an extension of .html (which, admittedly, is quite a bummer), and looks like:

 source/_patterns/atoms/general/brand-colors.html

Currently, this results in this:

 long-filename-here.html
 long-filename-here.html
 long-filename-here.markup-only.html

And that's the problem. The raw template version (the second file in that list) blows away the first one, since they wind up named the same. So, the idea is that if I provide this configuration:

outputFileSuffixes:
 - rendered: '.rendered'
 - rawTemplate: ''
 - markupOnly: '.markup-only'

I will instead wind up with a workable set of output files:

 long-filename-here.rendered.html
 long-filename-here.html
 long-filename-here.markup-only.html

For a template engine like Handlebars with a more sensible file extension, it will still work fine:

 long-filename-here.rendered.html
 long-filename-here.hbs
 long-filename-here.markup-only.html

Including the file extensions in the config assumes that all templates in a tree have the same extension, which is the case of Node is not necessarily true because we can have mixed pattern trees.

@dmolsen
Copy link
Member

dmolsen commented Jul 21, 2016

Ok, clearly overthinking. Then back to my original proposal/implementation. I appreciate the clarification @geoffp .

@dmolsen dmolsen closed this as completed Jul 21, 2016
@dmolsen
Copy link
Member

dmolsen commented Jul 22, 2016

implementation tips:

  1. don't fuck with node's export of the pattern paths object at all.
  2. make sure config.outputFileSuffixes.rendered is outputted. i need it for a part of urlHandler.

I'll update if there are any other tips. I think this is implemented for PHP and FE but want to test some more.

@geoffp
Copy link
Author

geoffp commented Jul 22, 2016

No worries. Thanks again!

@dmolsen
Copy link
Member

dmolsen commented Jul 22, 2016

Implemented with PL/PHP Core v2.7.0 and SAD v3.3.0.

@bmuenzenmeyer
Copy link
Member

@geoffp can you test with patternlab-node#dev and ensue this meets your needs?

  • npm update to get Dave's new frontend
  • check out the new config options
  • npm link to patternlab-node#dev

@dmolsen
Copy link
Member

dmolsen commented Jul 29, 2016

@bmuenzenmeyer -

In pattern-lab/styleguidekit-assets-default#39 a user notes that there's an issue with patternData.patternExtension having a . at the beginning of the extension when using the node version of Pattern Lab. Can we standardize on patternData.patternExtension being outputted as the file extension sans a leading period? e.g. mustache instead of .mustache.

@bmuenzenmeyer
Copy link
Member

@bmuenzenmeyer
Copy link
Member

I am also trying to ping that user back - as I think that code has now stabilized between PL/Node Core and SAD.

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

No branches or pull requests

3 participants