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

Format (registerFormat) vs Templates (registerTemplate) #126

Closed
didoo opened this issue Sep 6, 2018 · 13 comments
Closed

Format (registerFormat) vs Templates (registerTemplate) #126

didoo opened this issue Sep 6, 2018 · 13 comments

Comments

@didoo
Copy link
Contributor

didoo commented Sep 6, 2018

While working on #121 I have realized that in order to update the generation of the files (adding a comment text to the generated files when the user added it as a prop in the token) I had to both touch the template files and the formatting functions.

Now, given how easy it would be to create distinct template files for the formats that are contained in formats.js, I don't understand why this double way of producing the output. Wouldn't it be better to have a single way to generate the output files, with a single place where the user can see all the templates that are used to generate the output? Like, the first time I saw this library, I could not understand where the other templates for Sass were, or the PLIST for iOS, etc.

In other words, what is the reason for this double pipeline? And what are the reasons for not converting everything to a single way?

@dbanksdesign
Copy link
Member

We've definitely heard this feedback before. The short, not-so-good, answer is to give users options. Sometimes it might be easier to write a template rather than a formatter function. But... technically all templates could just be formats and the formatter function could just call the lodash template, which is kind of what happens when you call a template anyways. So yea, there isn't really much of a good reason to carry both around.

Maybe in the next major release (3.0) we drop templates, or provide a warning that templates are going away, and migrate all built-in templates to formats. We wouldn't need to actually rewrite them, just to make formatter functions that call the lodash template like so:

'ios/plist': _.template( fs.readFileSync(/* template file */) )

@didoo
Copy link
Contributor Author

didoo commented Sep 6, 2018

I see your point. What I was referring to was about the fact that as a user (or contributor), if I have to add a new "output format" or customize an existing one, I am a little bit lost, because of this ambiguity in "where" things are, and the inconsistency in the way/place where one declares this new/updated format. Maybe we could use a template file for all the formats (so also for scss, es6, and so on) and store them all in the same place/folder; and for all of them - as you suggest above - read them and process them in the format.js file. Would it make sense?

@chazzmoney
Copy link
Collaborator

This conversation seems similar to @elliotdickison suggestion to move to a more babel-like system. I like the idea of having everything in a single place so it can be found easily; I too struggled originally in trying to figure out where things were happening to generate the output files.

@dbanksdesign
Copy link
Member

I think this is a great discussion. I'm thinking the 3.0 release should tackle 2 major simplifications in the build process:

  • Transforms and resolving references: Handling them "inline", rather than doing all transforms first, then all reference replacements after.
  • Formats: This discussion here. Maybe getting rid of 'templates' and being more consistent in location and how formats are used.

@didoo
Copy link
Contributor Author

didoo commented Sep 7, 2018

@dbanksdesign when you say "Maybe getting rid of 'templates'" you mean getting rid of the templates as a way to generate output alternative to the format.js, or getting rid of the template files (read lodash or other similar templating) as a way to declare the template of the output, and declare the "template" as JS code directly? In the first case, I don't have strong opinions (I don't know enough the inner workings of the library) but on the second I can tell you that to me it was a very low barrier to start to play and tweak the template files and see what happened in output, and little by little tune them to get exactly the output I wanted. The template files are a low-barrier way to show to new users how the files are generated/formatted, compared to a more intimidating long JS file like formats.js.

@chazzmoney
Copy link
Collaborator

Hmmm... Best way to start this?

Should I create a 3.0 branch? Or a remove-formats branch?

@didoo
Copy link
Contributor Author

didoo commented Sep 26, 2018

@chazzmoney re. the branch naming, I don't have a strong opinion, you and @dbanksdesign know better the "flow" of this (amazing) project. so I'll leave it to you the decision.
re. the actual implementation, it would be great if you could highlight (broadly) what you intend to do, so I can try to give you some feedback (in my current implementation in the Cosmos Design System for Badoo, everything is a custom template now, they're so easy to create, modify, customise, etc.)
also, as soon as we have snapshot testing of the generated files (still need your feedback on #133), we can use it to refactor the formats/templates being sure that the output remains the same.

@chazzmoney
Copy link
Collaborator

My plan was to do what Danny described in his original comment:

Maybe in the next major release (3.0) we drop templates, or provide a warning that templates are going away, and migrate all built-in templates to formats. We wouldn't need to actually rewrite them, just to make formatter functions that call the lodash template like so:

'ios/plist': _.template( fs.readFileSync(/* template file */) )`

@dbanksdesign should I start a 3.0 branch for this?

@didoo didoo changed the title Formats vs Templates Format (registerFormat) vs Templates (registerTemplate) Sep 27, 2018
@didoo
Copy link
Contributor Author

didoo commented Sep 27, 2018

@chazzmoney ok, now I get it. it's not about ditching the templates as "files" compiled via _.template() (all my custom formats are now "template" files!) but to get rid of the double way registerFormat vs registerTemplate. ok, makes totally sense to me 👍

@dbanksdesign
Copy link
Member

Exactly. And @chazzmoney yea lets start a 3.0 branch, and lets also start adding stuff to the 3.0 project: https://github.com/amzn/style-dictionary/projects/3

@chazzmoney
Copy link
Collaborator

@didoo I created a PR (referenced above) to try to solve this confusion. Would love your thoughts!

@didoo
Copy link
Contributor Author

didoo commented Oct 4, 2018

@chazzmoney reviewed your PR and left a few small comments, but apart from that: great job!

@chazzmoney
Copy link
Collaborator

The is now resolved in 2.5 - Thank you!

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

No branches or pull requests

3 participants