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

[WIP] feat: switch engine-twig to use twing rather than node-twig #1135

Merged
merged 5 commits into from
Apr 3, 2020
Merged

[WIP] feat: switch engine-twig to use twing rather than node-twig #1135

merged 5 commits into from
Apr 3, 2020

Conversation

ringods
Copy link
Contributor

@ringods ringods commented Mar 12, 2020

Closes #285

Summary of changes:

Todo:

  • a lot of testing

@renestalder
Copy link
Contributor

renestalder commented Mar 12, 2020

@ringods Yes, I would be able to test the changes, I probably need some guidance here. For context, the pattern library I have is based on the PHP Twig Engine version.

So I would need to know:

  • Any particular upgrade/migration hints between my PHP patternlab version and the JS version (maybe you just know where I have to look)
  • Anything else from the standard JS/node based JavaScript Patternlab version to your changes I have to apply.

As I'm quite out of the loop what happened in Patternlab since back then.

Are you able to provide me with some references and hints that help me?

@ringods
Copy link
Contributor Author

ringods commented Mar 12, 2020

@renestalder first of all, thank you for stepping in with some help.

I am working on a local, but minimal setup which I will probably push to my own Github account shortly. That should be a setup that you can replicate with your pattern library.

I'll ping you when it is available and we can go over the setup together if needed.

@ringods
Copy link
Contributor Author

ringods commented Mar 13, 2020

@bmuenzenmeyer @sghoweri I could use some input on PL internals. I have updated the engine-twig to use the twig library rather than node-twig.

On my branch (in this PR), I added development-edition-engine-twig to ease testing the Twig engine development. In development-edition-engine-twig, I added the starterkit-twig-demo patterns, but changed all includes, extends and embed references from Twig namespaces to PL pattern names. So not:

{% includes "@templates/02-blog.twig" %}

but

{% includes "templates-blog" %}

With this in place, a yarn pl:build gives me a clean build output:

$ yarn pl:build
yarn run v1.22.4
$ patternlab build --config ./patternlab-config.json
Pattern Lab Node v5.7.0
Pattern Engine mustache: good to go
Pattern Engine handlebars: good to go
Pattern Engine mustache: already loaded, skipping.
Pattern Engine twig: good to go
Built pattern: atoms-colors
Built pattern: atoms-fonts
Built pattern: atoms-animations
Built pattern: atoms-visibility
Built pattern: atoms-headings
Built pattern: atoms-paragraph
Built pattern: atoms-blockquote
Built pattern: atoms-inline-elements
Built pattern: atoms-time
Built pattern: atoms-preformatted-text
Built pattern: atoms-hr
Built pattern: atoms-unordered
Built pattern: atoms-ordered
Built pattern: atoms-definition
Built pattern: atoms-logo
Built pattern: atoms-landscape-4x3
Built pattern: atoms-landscape-16x9
Built pattern: atoms-square
Built pattern: atoms-avatar
Built pattern: atoms-icons
Built pattern: atoms-loading-icon
Built pattern: atoms-favicon
Built pattern: atoms-text-fields
Built pattern: atoms-select-menu
Built pattern: atoms-checkbox
Built pattern: atoms-radio-buttons
Built pattern: atoms-html5-inputs
Built pattern: atoms-buttons
Built pattern: atoms-table
Built pattern: atoms-video
Built pattern: atoms-audio
Built pattern: molecules-byline
Built pattern: molecules-address
Built pattern: molecules-heading-group
Built pattern: molecules-blockquote-with-citation
Built pattern: molecules-intro-text
Built pattern: molecules-one-up
Built pattern: molecules-two-up
Built pattern: molecules-three-up
Built pattern: molecules-four-up
Built pattern: molecules-media-block
Built pattern: molecules-block-headline-byline
Built pattern: molecules-block-hero
Built pattern: molecules-block-thumb-headline
Built pattern: molecules-block-headline
Built pattern: molecules-block-inset
Built pattern: molecules-figure-with-caption
Built pattern: molecules-search
Built pattern: molecules-comment-form
Built pattern: molecules-newsletter
Built pattern: molecules-primary-nav
Built pattern: molecules-footer-nav
Built pattern: molecules-breadcrumbs
Built pattern: molecules-pagination
Built pattern: molecules-tabs
Built pattern: molecules-social-share
Built pattern: molecules-accordion
Built pattern: molecules-single-comment
Built pattern: molecules-alert
Built pattern: organisms-header
Built pattern: organisms-footer
Built pattern: organisms-article-body
Built pattern: organisms-comment-thread
Built pattern: organisms-latest-posts
Built pattern: organisms-recent-tweets
Built pattern: organisms-related-posts
Built pattern: templates-site
Built pattern: templates-page-1col
Built pattern: templates-page-2col
Built pattern: templates-homepage
Built pattern: templates-blog
Built pattern: templates-article
Built pattern: templates-article-2col
Built pattern: pages-homepage
Built pattern: pages-homepage-emergency
Built pattern: pages-blog
Built pattern: pages-article
Omitting atoms-video from styleguide patterns because it has an underscore prefix.
Omitting atoms-audio from styleguide patterns because it has an underscore prefix.
Omitting pages-homepage from styleguide patterns because it is defined as a defaultPattern.
Built Pattern Lab front end
⊙ patternlab → build: Yay, your Pattern Lab project was successfully built ☺
✨  Done in 1.58s.

But when I look at the output, I have a lot of rendered pattern HTML files only having NaN as content. Also, my top level index.html file does not seem to be processed as it still contains unprocessed tags.

Do I still have anything misconfigured?

@ringods
Copy link
Contributor Author

ringods commented Mar 19, 2020

@renestalder I have got something working. This PR contains 3 sets of changes:

  • I added package development-edition-engine-twig in this monorepo which can be used for local testing
  • I updated engine-twig to another twig engine. I first tried with the twig library but eventually got it working with the twing library. This is a pure Javascript engine, so no PHP involved.
  • I made a few fixes to the patterns in starterkit-twig-demo. Without these, no engine would have been able to work correctly

You can clone from my fork here: https://github.com/ringods/patternlab-node/tree/feature/twigjs

I suggest that you first try to get the development-edition-engine-twig working on your local workstation. This monorepo requires yarn rather than `npm, so make sure you have that installed.

$ yarn install
$ cd packages/development-edition-engine-twig
$ yarn run patternlab install --starterkits @pattern-lab/starterkit-twig-demo
$ yarn pl:build

This should give you a clean build output. The code should support twig relative includes, support for namespaced includes as well as based on the Patternlab pattern names (e.g. templates-blog).

Once you got this working, you can replace the patterns from the starterkit with your extensive library. Awaiting your results. ;-)

@ringods ringods changed the title [WIP] feat: switch engine-twig to use twig rather than node-twig [WIP] feat: switch engine-twig to use twing rather than node-twig Mar 19, 2020
@renestalder
Copy link
Contributor

@ringods Thank you very much for your time and work. I'll try to approach this as soon as possible, maybe today or next week. Don't hesitate to ping me again if you don't hear anything from me for a longer time.

@sghoweri
Copy link
Contributor

Hey there 👋 Just two quick meta comments I wanted to throw out there:

  1. Although part of me wishes that we could could get by with only a single Twig-based rendering engine, I can't argue with how nice it would be to be able to run everything in PL without any PHP dependencies (without giving up on the tons of benefits we get w/ Twig templates)... With that said, if we were going to continue to have and maintain a 2nd Twig based rendering engine in Pattern Lab, Twing would easily be my top choice to use. 👍

  2. Ideally I'd really like to see if a JavaScript-compiled Twig engine (Twing) and PHP-compiled Twig engine could both use the exact same set of starter Twig templates. I'd really hate to see us have to maintain two completely separate sets of Twig templates that should theoretically be 100% identical (custom Twig extensions aside)...

@ringods
Copy link
Contributor Author

ringods commented Mar 21, 2020

@sghoweri as I didn't grow up with English as my main language, I'm not sure how to interpret your first comment. Is it a statement or more of a question? Just checking because I first tried with twig as the JS engine, but because it didn't really work as I wanted it, I switched to twing and got it integrated quite nicely if I may say so myself. 😉

Regarding your second comment: I tested my work with starterkit-twig-demo. I only have to make this change (8250fe8) to the patterns. These were clear mistakes in the patterns which IMO wouldn't work in the PHP engine either. If we stick to that starterkit (and extend the patterns where needed), testing both engines should be a breeze.

@sghoweri
Copy link
Contributor

@sghoweri as I didn't grow up with English as my main language, I'm not sure how to interpret your first comment. Is it a statement or more of a question? Just checking because I first tried with twig as the JS engine, but because it didn't really work as I wanted it, I switched to twing and got it integrated quite nicely if I may say so myself. 😉

@ringods that's quite alright (and for what it's worth, I think your English is totally fine!) -- what I meant to say is that I would have picked Twing as well, good choice 👍

@ringods
Copy link
Contributor Author

ringods commented Mar 22, 2020

@sghoweri nice to hear I made a good choice eventually.

I just updated the README of the engine. Can you inform me how I should update the CHANGELOG.md? Or is this done automatically based on the commit info? Do I have to rewrite the git history to better reflect what changed?

@bmuenzenmeyer
Copy link
Member

Can you inform me how I should update the CHANGELOG.md? Or is this done automatically based on the commit info? Do I have to rewrite the git history to better reflect what changed?

the changelog is automatically created - scanning the commit names you have here - everything should be fine from what I can tell

@ringods
Copy link
Contributor Author

ringods commented Mar 27, 2020

@renestalder kindly reminder. Awaiting results of testing with your private twig patterns. 😉

@ringods
Copy link
Contributor Author

ringods commented Mar 27, 2020

@sghoweri @bmuenzenmeyer can we decouple further twig testing from integrating this work into master and into a new release? Current testing has been performed with starterkit-twig-demo and works.

I am asking this because I would like to base my own setups on top of my twig work. 😉

@ringods
Copy link
Contributor Author

ringods commented Apr 2, 2020

@sghoweri @bmuenzenmeyer can you merge & release this?

@bmuenzenmeyer bmuenzenmeyer merged commit 749f648 into pattern-lab:dev Apr 3, 2020
@bmuenzenmeyer
Copy link
Member

@ringods thanks for carrying this forward! - I've just added you to the trusted committer team in the org.

@renestalder
Copy link
Contributor

Sorry for not being able to test it yet. I'm a professional procrastinator.

@ringods
Copy link
Contributor Author

ringods commented Apr 3, 2020

@bmuenzenmeyer thank you for the trust in me.

@ringods
Copy link
Contributor Author

ringods commented Apr 4, 2020

@renestalder if you want to test, you can use the released version of the package:

https://www.npmjs.com/package/@pattern-lab/engine-twig/v/5.8.0

Please create new issues if you find bugs. 😉

antonia-rose pushed a commit to quelltexterin/nemo-uikit-workshop that referenced this pull request Apr 12, 2023
[WIP] feat: switch engine-twig to use twing rather than node-twig
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.

Verify maturity of Twig engine
4 participants