Skip to content

Commit

Permalink
fix: update twig support from github.com//pull/129
Browse files Browse the repository at this point in the history
  • Loading branch information
@rejas
rejas committed Mar 29, 2018
1 parent f69bc70 commit 831d4b0
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion src/transform/index.js
View file
Expand Up @@ -4,7 +4,7 @@ var extname = require('../extname');
/**
* Constants
*/
var TARGET_TYPES = ['html', 'html.twig', 'jade', 'pug', 'slm', 'slim', 'jsx', 'haml', 'less', 'sass', 'scss'];
var TARGET_TYPES = ['html', 'jade', 'pug', 'slm', 'slim', 'jsx', 'haml', 'less', 'sass', 'scss', 'twig'];
var IMAGES = ['jpeg', 'jpg', 'png', 'gif'];
var DEFAULT_TARGET = TARGET_TYPES[0];

Expand Down Expand Up @@ -194,6 +194,14 @@ transform.scss.sass = transform.less.less;
transform.scss.scss = transform.scss.sass;
transform.scss.css = transform.scss.sass;

transform.twig.css = function (filepath) {
return '<link rel="stylesheet" href="{{ asset("' + filepath + '") }}"' + end();
};

transform.twig.js = function (filepath) {
return '<script src="{{ asset("' + filepath + '") }}"></script>';
};

/**
* Transformations for jsx is like html
* but always with self closing tags, invalid jsx otherwise
Expand Down

12 comments on commit 831d4b0

@michaellenahan
Copy link

@michaellenahan michaellenahan commented on 831d4b0 Apr 5, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I just wanted to let you know that this commit caused a problem (way) downstream. This is just for info, not to give blame. I know my project is not managing dependencies in a perfect way, to say the least. More details are here: https://github.com/michaellenahan/blog/wiki/PHP-Fatal-error:--Uncaught-Twig_Error_Syntax:-Unknown-%22asset%22-function

@rejas
Copy link
Contributor Author

@rejas rejas commented on 831d4b0 Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @michaellenahan thx for the info. I dont have too much experience with twig so I ask you: would it be better for the general public to inject only into .html.twig files (and are .twig files like in your example partial html tempaltes that get injected into a final html.twig file?=

@michaellenahan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak for the "general public", my expertise is more with back-end development. What I can say is: if you only injected into .html.twig files that would certainly help me, it would not break my deployment.

@rejas
Copy link
Contributor Author

@rejas rejas commented on 831d4b0 Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does your gulp taks look like? Are the src only *.twig files or can you use *.html.twig to seperate those from the non-wanted files?

@michaellenahan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source file where the unwanted injection occurred was called _01-foot.twig the problem was caused because suddenly *.twig files were amended instead of as before, *.html.twig

So, if you go back to just amending *.html.twig files that would fix my particular issue.

@michaellenahan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For info, I've just cross-posted this issue to the issue queue of the pattern lab drupal theme - that's what I'm using in my Drupal site that has a dependency on gulp-inject: phase2/pattern-lab-drupal-theme#7

@rejas
Copy link
Contributor Author

@rejas rejas commented on 831d4b0 Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you post your gulp code nonetheless?

@alexdruhet
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, to my sense it's not a good idea to verticalize the twig injection trough the asset function. This function is not part of Twig core, it's a Symfony extension (https://symfony.com/doc/3.4/reference/twig_reference.html#asset). A project can use Twig without Symfony!

@rejas
Copy link
Contributor Author

@rejas rejas commented on 831d4b0 Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pixadelic since I dont understand all the technical stuf in your post: is that an advice to @michaellenahan ?

@alexdruhet
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if it's unclear but my comment was for you. I redo in a more correct way: transform.twig.* will wrap automatically the injected string in an asset() function. That the point I was discussing because the asset() function is not bundled with Twig package (as seen at https://twig.symfony.com/doc/2.x/), but it's part of the Symfony framework.

Hence it's possible to use twig without having asset() function available. So the new gulp-inject twig feature is nice but can break some task processes. But it's not a big deal, thank a lot for this module!

@gligoran
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this wrapping feature opt-in or at least opt-out? The wrapping into asset function is breaking my deploy process, because Symfony is apparently checking weather the files actually exist. Because of some cache-busting I'm doing along side injecting files into my twigs, they don't actually exist and deploy fails.

For now I'm reverting to 4.3.1.

@rejas
Copy link
Contributor Author

@rejas rejas commented on 831d4b0 Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would glady accept any PR to make this feature opt-out and get your progress back on track. COuld you make one for this or do you need help?

Please sign in to comment.