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

initial support for jade/pug templates #2205

Closed
wants to merge 2 commits into from
Closed

Conversation

ShadowManu
Copy link

@ShadowManu ShadowManu commented Sep 18, 2016

This is a PR regarding #1886. It doesn't include documentation changes (this is to allow using .pug files in components the same way we already can use .scss files on them), but its a start to know what you guys think / what is missing. Thanks.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@ShadowManu
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@filipesilva
Copy link
Contributor

See #1886 (comment)

@niklas-dahl
Copy link
Contributor

@ShadowManu how is it going? are you going to add documentation/e2e tests soon?

thanks, looking forward to this getting merged :D

@ShadowManu
Copy link
Author

I actually could have time today or this week, I haven't forgot. Just had a busy one ;)

@ShadowManu ShadowManu force-pushed the pug branch 2 times, most recently from d65009d to 0681292 Compare October 15, 2016 07:03
@KiaraGrouwstra
Copy link

Hm, maybe with pug-html-loader over pug-loader the apply-loader would no longer be needed?

By default using Pug in the Angular2 context also brings a few pain points compared to using HTML:

  • Angular template variable e.g. #myVar would need to become #myVar=''; similarly directives like md-raised-button would need to become md-raised-button=''. These cases we could solve by passing doctype: 'html' into the Pug settings.
  • [prop] and (event) bindings must be quoted or separated from other attributes using commas. This point can be addressed using plugins: [require('pug-plugin-ng')].

Might those perhaps be useful since our Pug use-case specifically pertains Angular2 here?

@ShadowManu
Copy link
Author

Answering from phone, so forgive my lack of links (and of course the autocorrects)

  1. Using pug-html-loader: the project doesn't have many stars, it's not officially maintained by the pug community (afaik). You could say the same about the apply loader, but that's something simple and that can be worked around in the original pug loader in the future. So I'm not in pro of changing the loader.

  2. Html 5 mode: using attributes without value in pug will (by default) output stuff like attr=attr. This can be 'fixed' by setting the doctype both as a header per pug file (ad I normally do) or as a global setting as you said. It's an opinionated default (to which I agree, pertains to Angular 2) if we set it.

  3. Angular syntax requiring comma-separated or inside quote attributes: the angular-used characters for attributes requires one of the mentioned options. The plugin sounds cool but goes against pug chosen behavior. I would prefer a documented usage and add it also to angular cli Readme, than offer a custom behavior not accepted because of reasons. Either way, same thing about the official support by pugjs.

@KiaraGrouwstra
Copy link

KiaraGrouwstra commented Oct 19, 2016

On the plugin, one point I might add is it doesn't so much hurt the default notation (as one might use without it), but yeah. I do agree allowing overall customization would be preferable.

Perhaps I should check if ng-cli already allows overriding parts of its webpack config... so far from the documentation I hadn't been under the impression this was currently possible though. But overrides could potentially take care of other use-cases as well.

Heck, people would no longer need to wait for ng-cli to accept PRs for specific additions, while ng-cli would work out by delegating feature addition to webpack without taking on additional dependencies.

@niklas-dahl
Copy link
Contributor

@ShadowManu any updates on this pr?

IMO setting the html doctype as a global default should be done for angular projects.
but I'm not quite sure on using pug-plugin-ng..

@Ionaru Ionaru mentioned this pull request Nov 23, 2016
@ishitatsuyuki
Copy link
Contributor

+1 on this. A neat fix and seems ready for merge.

@ishitatsuyuki
Copy link
Contributor

Something is wrong with index.html. The output is being index.pug after Ι changed the config, and ng serve no longer works.

@randallmeeker
Copy link

We are currently using this PR with the knowledge that it only affects templates and not index.html. Happy to see it merged in the short term and then another PR to make index.pug run through the parser.

@@ -96,6 +96,7 @@ export function getWebpackCommonConfig(
       { test: /\.json$/, loader: 'json-loader' },
       { test: /\.(jpg|png|gif)$/, loader: 'url-loader?limit=10000' },
       { test: /\.html$/, loader: 'raw-loader' },
{ test: /\.(pug|jade)/, loaders: ['apply-loader', 'pug-loader']},

Choose a reason for hiding this comment

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

In the event one you'd want to add configuration here, it seems that could be done as follows:

       {
          test: /\.(pug|jade)$/,
          loader: 'apply',
       },
       {
          test: /\.(pug|jade)$/,
          loader: 'pug',
          query: { doctype: 'html' },
       },

Earlier I feared for my plugin thing I'd need to use the single loader instead (pug-html), but it seems that'd work fine with your setup as well.

@randallmeeker
Copy link

is there something specific this that is blocking this PR. I'm happy to pitch in.

@ishitatsuyuki
Copy link
Contributor

  • Merge conflict
  • Pug isn't just a syntax alternative but also used in dynamic template engines. A little bit controversial
  • To add the pug/ng syntax plugin or not.

@KiaraGrouwstra
Copy link

I'd gladly defer plugin discussion to a separate thread, since it shouldn't be blocking initial support.

Dynamic templating, I've made a bit of use of even in the Angular 2 context so far, but for the most part you'd want to relegate this logic to Angular. I did like the ability to include separate files so as to allow breaking things down a little (without necessarily having to use distinct components). In my experience, the added functionality may find a bit of occasional use but otherwise doesn't really hurt when not using it. Obviously the primary added value of Pug in the Angular context is just nicer syntax.

I'd help resolve the merge conflict, but I fear the PR isn't mine. :P

@ishitatsuyuki
Copy link
Contributor

This is too initial; it may just confuse newcomers. No ng generate templates are provided, and it has the index problem I mentioned before.

Regarding pug itself: unnecessary functionality, it's syntax sugar can be easily replaced by emmet.

@randallmeeker
Copy link

randallmeeker commented Dec 1, 2016

There seem to be two conflicting points of contention.

A.) This PR is incomplete
B.) Pug is not a feature this repo want baked in (possibly in a plugin, but not this PR)

I want it and am happy to help finish it, but will not probably get into the weeds if is to be a rejected feature. I don't see finishing this as a huge hurdle (docs, ng generate ect ...) so I think the only conversation to be had is point B.)

Regarding pug itself: unnecessary functionality, it's syntax sugar ...

This same over generalized argument can be made for sass or typescript really.

@niklas-dahl
Copy link
Contributor

Regarding Point B:
since this PR almost went through #626 I guess the cli team has nothing against adding pug support.
And there are apparently a lot of people (including me) who want pug supported..

@KiaraGrouwstra
Copy link

@niklas-dahl: this makes some sense; the Angular team does in fact like it themselves. :)

@KiaraGrouwstra
Copy link

Hi @ShadowManu, in anticipation of getting basic support in I tried to see if I could get a little progress here. I couldn't make a PR to your fork as I already had a fork of the original of my own, but I tried to go from your branch to resolve merge conflicts and add a little documentation/testing, see here.
That said I've been a bit confused w.r.t. how to run the tests; I got a Unexpected token import on the other tests already so I'm probably doing things wrong.

@ShadowManu
Copy link
Author

Sorry If I can't help that much, since I'm on travel. I tried to follow the advice Filipe gave he as a comment in the original issue, trying to copy tests for scss. You may have a problem using ES6 import where you can't, but I'm only guessing.

@KiaraGrouwstra
Copy link

@ShadowManu: I tried adding tests along those lines as well now. (I'm not sure what's up with running them -- the part that's failing wasn't my code.)
Might you perhaps be able to add my remote and merge it in so as to gather more feedback from people/CI here?

@jurienhamaker
Copy link

Any news on this?

@filipesilva
Copy link
Contributor

Closing, please see #1886 (comment) for context.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants