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

Multiple conditions not working with `expressionLanguage => 'js'` option. #216

Closed
narirou opened this issue Jan 7, 2019 · 8 comments
Closed

Comments

@narirou
Copy link

@narirou narirou commented Jan 7, 2019

Hello,

I encountered an issue with the following code with rendering option expressionLanguage => 'js':

doctype html
html
  body
    if falsy || !falsy || !falsy
      p 1
    else
      p 2

with data:

array(
  'falsy' => false
)

I expected to get:

<!DOCTYPE html>
<html>
  <body>
    <p>1</p>
  </body>
</html>

But I actually get:

<!DOCTYPE html>
<html>
  <body>
    <p>2</p>
  </body>
</html>

js-phpize-phug 2.x will occer this error, but 1.x not occerred.

Thanks!

@kylekatarnls

This comment has been minimized.

Copy link
Member

@kylekatarnls kylekatarnls commented Jan 7, 2019

Hi, this a side effect of handling myVariable || "default value"

You can wrap:

doctype html
html
  body
    if falsy || (!falsy) || (!falsy)
      p 1
    else
      p 2

Or force js-phpize booleanLogicalOperators option:

<?php

include __DIR__.'/vendor/autoload.php';

$pug = new Pug([
    'module_options' => [
        'jsphpize' => [
            'booleanLogicalOperators' => true,
        ],
    ],
]);
echo $pug->renderString('doctype html
html
  body
    if falsy || !falsy || !falsy
      p 1
    else
      p 2
', [
  'falsy' => false,
]);

It might be handled in a next major version of https://github.com/pug-php/js-phpize but not planned yet.

@kylekatarnls

This comment has been minimized.

Copy link
Member

@kylekatarnls kylekatarnls commented Jan 7, 2019

We're still open to pull-request if it can get both (the mixed type result and the correct precedence):

shouldBeTrue = false || !false
shouldBe42 = false || 42
@narirou

This comment has been minimized.

Copy link
Author

@narirou narirou commented Jan 7, 2019

Thanks for quick answer.
Changing the condition requires modifying a lot of code so far, so we will try to change this options instead.

@kylekatarnls

This comment has been minimized.

Copy link
Member

@kylekatarnls kylekatarnls commented Jan 7, 2019

The difference you should check is everywhere you have false || foo you will now get true while you get previously foo, then true && foo will now give !!foo instead of foo. Then you get the correct precedence between && / || and other operators like !.

@matthewjumpsoffbuildings

This comment has been minimized.

Copy link

@matthewjumpsoffbuildings matthewjumpsoffbuildings commented Sep 16, 2019

Is this issue documented anywhere? Because just ran into this, and thought we were going crazy, and now we are worried about this issue happening all over the place and we just didnt realise...

You just kinda expect multiple conditions to work as normal, and when it doesnt its super confusing, probably should be a big warning in the readme

@kylekatarnls

This comment has been minimized.

Copy link
Member

@kylekatarnls kylekatarnls commented Sep 16, 2019

You can propose your modification to the documentation here: https://github.com/phug-php/website
(or edit buttons in https://www.phug-lang.com/) but this certainly does not deserve a big blinking red warning.

@matthewjumpsoffbuildings

This comment has been minimized.

Copy link

@matthewjumpsoffbuildings matthewjumpsoffbuildings commented Sep 16, 2019

@kylekatarnls

This comment has been minimized.

Copy link
Member

@kylekatarnls kylekatarnls commented Sep 16, 2019

Thanks. It's not that easy, but allowing the JS-like cast (false || 42 giving 42) + the correct operators precedence is still in the pipe for https://github.com/pug-php/js-phpize. And it's open to pull-requests if someone want to give a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.