Skip to content

Conversation

@cossssmin
Copy link
Member

This PR fixes a couple of bugs when ignoring expressions with @{{ }}, as described in #72.

Subsequent ignored expressions now properly work in text nodes:

<p>Here's one @{{ variable }} and here's @{{ another }}.</p>

Result:

<p>Here's one {{ variable }} and here's {{ another }}.</p>

Expressions are now also ignored inside attribute values:

<p data-foo="@{{ bar }}">Lorem ipsum.</p>

Result:

<p data-foo="{{ bar }}">Lorem ipsum.</p>

When merged, this PR will close #72.

@cossssmin cossssmin requested review from Scrum and anikethsaha May 9, 2020 12:15
@coveralls
Copy link

coveralls commented May 9, 2020

Coverage Status

Coverage decreased (-0.05%) to 96.891% when pulling 28ad6ac on cossssmin:fix-ignored into 77aaf56 on posthtml:master.

@cossssmin cossssmin requested a review from Scrum May 15, 2020 12:32
@Scrum
Copy link
Member

Scrum commented May 15, 2020

@cossssmin Hi, why didn’t you move it to line 154? after all, during the round-trip, regular degeneration with ignore

@cossssmin
Copy link
Member Author

Rushed to push the fix, sorry about that - doing it now... 🤦‍♂️

@Scrum
Copy link
Member

Scrum commented May 15, 2020

Don't worry about it

@cossssmin
Copy link
Member Author

Wait, it won't work there, delimitersSettings isn't yet defined... did you mean line 164?

@Scrum
Copy link
Member

Scrum commented May 15, 2020

Yes, I was a little mistaken, but I would like to endure them or iterate walk, but I see for this they need to be taken out of the osprey for visibility even higher somewhere 13 line

@cossssmin
Copy link
Member Author

delimitersSettings, which the ignoredBefore and ignoredAfter variables (which are the delimiters, {{ and }} by default) use, are not set at that point in time...

@Scrum
Copy link
Member

Scrum commented May 15, 2020

I mean, in line 13, initialize them so that they are accessible from any scoup

@cossssmin
Copy link
Member Author

Not sure what to initialize them to or how you'd like it to work, sorry... I'd appreciate your assistance if you can, feel free to update the PR :)

@Scrum
Copy link
Member

Scrum commented May 15, 2020

@cossssmin its ok ?

@Scrum
Copy link
Member

Scrum commented May 15, 2020

@anikethsaha ping

Comment on lines 2 to 3
<p data-username="@{{ user.name }}" data-user-id="user-@{{ user.id }}">
Here's one @{{ variable }} and here's @{{ another }}.
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking suggestion !

can we have one more example here with usual expression and one ignored expression on same element and also or the same attribute?

like this may

<p data-username="@{{ user.name }}" data-user-id="user-@{{ user.id }}-{{ user.id2 }}">
  And here's a @{{ variable }} that would be undefined.	  Here's one {{ variable }} and here's @{{ another }}.

ignore if already exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once @{{ is encountered, the entire string/attribute value is returned as-is.

So this:

Here's one @{{ variable }} and here's some {{ foo }}.

will be output as:

Here's one {{ variable }} and here's some {{ foo }}.

So {{ foo }} will not be evaluated. Which come to think about it, may not be ideal, but I can't see how you'd evaluate {{ foo }}...?

Copy link
Member

Choose a reason for hiding this comment

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

ummm,

can we do like if @{{ comes, it will ignore till first occurrence of }} ? WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder, maybe changing the regexes here:

const delimitersRegexp = new RegExp(`${before}(.+?)${after}`, 'g')
before = escapeRegexpString(options.unescapeDelimiters[0])
after = escapeRegexpString(options.unescapeDelimiters[1])
const unescapeDelimitersRegexp = new RegExp(`${before}(.+?)${after}`, 'g')
// make array of delimiters
const delimiters = [
{ text: options.delimiters, regexp: delimitersRegexp, escape: true },
{ text: options.unescapeDelimiters, regexp: unescapeDelimitersRegexp, escape: false }
]

... to match {{ }} or {{{ }}} as long as they don't start with @, would be better instead? So we only match non-ignored delimiters, and then we no longer have to return 'early', so my example above should (theoretically) correctly return:

Here's one {{ variable }} and here's some bar.

(assuming foo: 'bar' in locals).

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

to match {{ }} or {{{ }}} as long as they don't start with @, would be better instead?

yeah, I think this should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it working, will push it up in a moment 👍

Copy link
Member

Choose a reason for hiding this comment

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

can you try ^(?! )[^@]{(.+?)}} ?

Copy link
Member

Choose a reason for hiding this comment

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

great 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

So although I got it working, I think I found an issue with how placeholders treats unescaped delimiters.

Basically, since it loops over every item in the settings object it receives, it'll still match {{ var }} inside @{{{ var }}}.

I'll push the fix for regular, escaped delimiters and will open a separate issue for unescaped delimiters.

Copy link
Member

Choose a reason for hiding this comment

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

I'll push the fix for regular, escaped delimiters and will open a separate issue for unescaped delimiters.

cool 👍

@cossssmin
Copy link
Member Author

@cossssmin its ok ?

Yes, sorry, I got what you meant now... 👍

use negative lookbehind so we only evaluate non-ignored expressions
@cossssmin cossssmin requested a review from anikethsaha May 15, 2020 16:31
Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

🎉

@Scrum Scrum merged commit 4488115 into posthtml:master May 18, 2020
@Scrum Scrum mentioned this pull request May 18, 2020
8 tasks
@Scrum
Copy link
Member

Scrum commented May 18, 2020

@cossssmin publish v1.4.1

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.

@{{ }} not working as expected

4 participants