search

pugjs / babel-plugin-transform-react-pug

A plugin for transpiling pug templates to jsx
MIT License
811 stars 47 forks source link

Handle shorthands in attributes correctly #33

Closed ezhlobo closed 6 years ago

ezhlobo commented 6 years ago

It adds supporting of shorthands in attributes. Initial issue #1 (Fails on boolean attributes).

Now following syntax won't throw an error:

div(first second third)

But this one will throw:

div(
  first!="stirng" 
  second!=variable
)

Checklist:

ezhlobo commented 6 years ago

The main reason why I created this PR is to get feedback about my direction.

ForbesLindesay commented 6 years ago

Looks good. Ping me once you've fixed build & made the changes you want to make.

ezhlobo commented 6 years ago

@ForbesLindesay I rewrote my solution completely (left only test cases), need your attention one more time.

Also it looks like I need your help with publishing new version of pug-lexer (after #2956 and #2957). Otherwise we need to support a workaround in our codebase.

ForbesLindesay commented 6 years ago

I don't have a strong opinion on whether we should support div(data-whatever!="some string that does not contain any special characters") or not.

Please can you address the other small comments though.

ezhlobo commented 6 years ago

@ForbesLindesay thank you for your review. I addressed all your comments and removed my workaround because there is new version of pug-lexer (that made it for us).

The question about checking tags in scripts is still open. I would suggest to create a separate issue and discuss it there, because anyway it did not work as expected before this PR. Does it work for you? We can actually remember all use cases of unescaped values and think up how to handle them best...

ForbesLindesay commented 6 years ago

I'm not sure if it worked for me or not, I haven't actually made use of that bit anyway.

ezhlobo commented 6 years ago

@ForbesLindesay hey, I pushed updates according to our discussion. Need your attention one more time.

you've dropped the BooleanLiteral case there. Is that intentional?

Yes. Now all boolean attributes come to us with mustEscape: true, so it is not necessary for us.

But the following example will throw an error and it's intentionally:

div(name!=true)
ForbesLindesay commented 6 years ago

Thanks :)

ezhlobo commented 6 years ago

Thank you very much for your time and patience