Handle shorthands in attributes correctly #33

ezhlobo · 2018-02-12T20:38:33Z

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:

Use context.error instead of new Error to keep well-described errors
Update pug-lexer and remove workaround to keep mustEscape correct

ezhlobo · 2018-02-12T20:39:36Z

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

ForbesLindesay · 2018-02-13T17:53:56Z

src/visitors/Tag.js

@@ -57,8 +67,8 @@ function getAttributes(

      const expr = parseExpression(val === true ? 'true' : val, context);

-      if (!mustEscape && (!t.isStringLiteral(expr) || /(\<\>\&)/.test(val))) {


Why are you removing the (!t.isStringLiteral(expr) || /(\<\>\&)/.test(val))?

@ForbesLindesay because it looks impossible to achieve with basic tests. Maybe we can hack it somehow... Otherwise val starts and ends with " when it's a string.

And filtering off <>& looks not obvious for me, why not <script>... or something like that? Probably we lost g flag for regex?

Anyway I'm going to use different conditions. All use cases are captured in tests. Let's discuss something, if you want to add to those tests.

The case this was capturing, although arguably not very important, is:

div(data-whatever!="some string that does not contain any special characters")

The idea being that it might make it slightly easier when converting from html based pug templates to react pug templates. Arguably that might not be super important though.

ForbesLindesay · 2018-02-13T17:55:17Z

src/__tests__/attributes-shorthand.input.js

+'use strict';
+
+// To prevent warnings in console from react
+const Custom = () => null


You could also render an input(checked) so we can actually see html output in the test.

@ForbesLindesay I started using data- attributes to keep html output. Let me know if you think that it does not appropriate.

ForbesLindesay · 2018-02-13T17:56:05Z

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

ezhlobo · 2018-03-05T21:45:14Z

@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 · 2018-03-06T13:29:15Z

src/visitors/Tag.js

-        throw new Error('Unescaped attributes are not supported in react-pug');
+      if (!mustEscape) {
+        const isStringViaAliases: boolean =
+          t.isStringLiteral(expr) && !['className', 'id'].includes(name);


For a list of two, I think I'd prefer just (name === 'className' || name === 'id') - it takes less time to mentally parse.

ForbesLindesay · 2018-03-06T13:29:52Z

src/visitors/Tag.js

-      if (!mustEscape && (!t.isStringLiteral(expr) || /(\<\>\&)/.test(val))) {
-        throw new Error('Unescaped attributes are not supported in react-pug');
+      if (!mustEscape) {
+        const isStringViaAliases: boolean =


You shouldn't need to explicitly add the : boolean here; flow should infer it from context.

ForbesLindesay · 2018-03-06T13:29:58Z

src/visitors/Tag.js

+        const isStringViaAliases: boolean =
+          t.isStringLiteral(expr) && !['className', 'id'].includes(name);
+
+        const isNotStringOrBoolean: boolean =


ForbesLindesay · 2018-03-06T13:32:24Z

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.

Get rid of .includes method for array of 2 items

ezhlobo · 2018-03-06T15:50:29Z

@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 · 2018-03-06T16:36:42Z

src/visitors/Tag.js

+          t.isStringLiteral(expr) && name !== 'className' && name !== 'id';
+
+        const isNotStringOrBoolean =
+          !t.isStringLiteral(expr) && !t.isBooleanLiteral(expr);


I'm just wondering if this lot can be clarified. It seems like the allowed case is just:

t.isBooleanLiteral(expr) || ((name === 'className' || name === 'id') && t.isStringLiteral(expr))

It seems simpler written like that to me, rather than having multiple references to isStringLiteral. How about we just use:

const isAllowedUnescapedAttribute = ( t.isBooleanLiteral(expr) || ((name === 'className' || name === 'id') && t.isStringLiteral(expr)) ); if (!isAllowedUnescapedAttribute) {

@ForbesLindesay I agree that my naming can confuse, but it was created with following reason in mind: all not obvious conditions should be commented. Variables are more convenient than just comments, so I used them.

isAllowedUnescapedAttribute sounds not obvious because we don't use such naming through this plugin and pug (I mean it does not respect spirit of mustEscape). It's better to keep shouldBeEscaped, allowedToBeEscaped.

Let me show how did I achieve my code:

When attribute is marked that we can skip its escaping, we should always show a message about unsupported feature. Two exceptions, where we should not show a message:

if attribute is a string not from aliases (not from className and id)

if attribute is not primitive (in pug it's everything besides string and booleans)

If I convert it to the code:

if (!mustEscape) { const isStringNotFromAliases = t.isStringLiteral(expr) && name !== 'className' && name !== 'id'; const isNotPrimitive = !t.isStringLiteral(expr) && !t.isBooleanLiteral(expr); if (isStringNotFromAliases || isNotPrimitive) { throw context.error( 'INVALID_EXPRESSION', 'Unescaped attributes are not supported in react-pug', ); } }

This snippet is what I suggest to leave. But I don't mind to make it much more compact by refactoring javascript, not the logic. If this is the case, then could you suggest any concrete solution that I can use? Maybe you can help me with optimizing the logic?

I'd much rather see a whitelist of cases where it is ok to have unescaped attributes:

className and id if the value is a string literal

any attribute if the value is a boolean

As this is much easier for me to interpret. When I read your code, I have to mentally figure out which are the allowed cases. Since the allowed cases are so narrow, I never really feel like I need to figure out what the non-allowed cases are.

allowedToBeEscaped doesn't make sense because everything can be escaped. mustEscape is fine, but we already have that with a subtly different meaning. I think something like escapingNotRequired or canSkipEscaping makes more sense here.

I agree that variable names are generally preferable to comments, but it's even better if the code makes sense without needing to read the variable names. I think you've ended up with much more complex code here in an effort to give names to things.

@ForbesLindesay you are right that understanding whitelist is much better here. But I still need your help with figuring out the right way. Does following code look better:

if (!mustEscape) { const canSkipEscaping = (name === 'className' || name === 'id') && t.isStringLiteral(expr); if (!canSkipEscaping) { throw context.error( 'INVALID_EXPRESSION', 'Unescaped attributes are not supported in react-pug', ); } }

I noticed that we should not check is the value boolean or not. We don't allow to write != constructions at all. Could you please take a look at tests in this PR and tell me is everything appropriate there?

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

ForbesLindesay · 2018-03-06T16:37:49Z

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

ezhlobo · 2018-03-20T10:04:13Z

@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 · 2018-03-20T12:06:39Z

Thanks :)

ezhlobo · 2018-03-20T12:10:36Z

Thank you very much for your time and patience

ForbesLindesay reviewed Feb 13, 2018

View reviewed changes

ezhlobo added 6 commits March 5, 2018 21:30

Updates after pulling

44a69b1

Add tests for handling unescaped attributes

71b2f72

Add tests for attributes shorthands

1daa348

Add the possibility to handle attributes correctly

4d82ce9

Expect mustEscape set to false for boolean attributes

b7f9ac3

Consider that boolean attributes does not nead to be escaped

b2cf339

ezhlobo force-pushed the attr-shorthands branch from 8987fc7 to b2cf339 Compare March 5, 2018 19:41

ezhlobo added 3 commits March 5, 2018 23:35

Allow throwing context-related errors for inline code

6e910f5

Upgrade flow-bin

0eaaaf8

Show errors with referencing to context

dfd9315

ForbesLindesay reviewed Mar 6, 2018

View reviewed changes

ezhlobo added 3 commits March 6, 2018 18:20

Get rid of extra types

bb616d9

Make code more clear:

efb0089

Get rid of .includes method for array of 2 items

Update react dependencies and remove temporary workaround

5f9c761

ForbesLindesay reviewed Mar 6, 2018

View reviewed changes

ForbesLindesay mentioned this pull request Mar 7, 2018

Is it possible to set the docytype? #40

Closed

ezhlobo added 3 commits March 20, 2018 12:25

Add tests for numbers as a value of attribute

59f3f7f

Small refactoring: rename variables and simplify the logic

ecc69e9

Update dependencies to get new API of pug-parser

32e4f29

ForbesLindesay merged commit 3fb864e into pugjs:master Mar 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle shorthands in attributes correctly #33

Handle shorthands in attributes correctly #33

ezhlobo commented Feb 12, 2018 •

edited

ezhlobo commented Feb 12, 2018

ForbesLindesay Feb 13, 2018

ezhlobo Mar 5, 2018

ForbesLindesay Mar 6, 2018

ForbesLindesay Feb 13, 2018

ezhlobo Mar 5, 2018 •

edited

ForbesLindesay commented Feb 13, 2018

ezhlobo commented Mar 5, 2018

ForbesLindesay Mar 6, 2018

ForbesLindesay Mar 6, 2018

ForbesLindesay Mar 6, 2018

ForbesLindesay commented Mar 6, 2018

ezhlobo commented Mar 6, 2018

ForbesLindesay Mar 6, 2018

ezhlobo Mar 6, 2018

ForbesLindesay Mar 7, 2018 •

edited

ezhlobo Mar 19, 2018

ForbesLindesay Mar 19, 2018 •

edited

ForbesLindesay commented Mar 6, 2018

ezhlobo commented Mar 20, 2018

ForbesLindesay commented Mar 20, 2018

ezhlobo commented Mar 20, 2018

		@@ -57,8 +67,8 @@ function getAttributes(

		const expr = parseExpression(val === true ? 'true' : val, context);

		if (!mustEscape && (!t.isStringLiteral(expr) \|\| /(\<\>\&)/.test(val))) {

Handle shorthands in attributes correctly #33

Handle shorthands in attributes correctly #33

Conversation

ezhlobo commented Feb 12, 2018 • edited

ezhlobo commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezhlobo Mar 5, 2018 • edited

Choose a reason for hiding this comment

ForbesLindesay commented Feb 13, 2018

ezhlobo commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForbesLindesay commented Mar 6, 2018

ezhlobo commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Let me show how did I achieve my code:

ForbesLindesay Mar 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ForbesLindesay Mar 19, 2018 • edited

Choose a reason for hiding this comment

ForbesLindesay commented Mar 6, 2018

ezhlobo commented Mar 20, 2018

ForbesLindesay commented Mar 20, 2018

ezhlobo commented Mar 20, 2018

ezhlobo commented Feb 12, 2018 •

edited

ezhlobo Mar 5, 2018 •

edited

ForbesLindesay Mar 7, 2018 •

edited

ForbesLindesay Mar 19, 2018 •

edited