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

Handle shorthands in attributes correctly #33

Merged
merged 15 commits into from Mar 20, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions .flowconfig
Expand Up @@ -4,8 +4,10 @@
[include]

[libs]


./scripts/babel-nodes.js

[lints]

[options]

[strict]
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Expand Up @@ -25,7 +25,7 @@
"babel-plugin-transform-flow-strip-types": "^6.22.0",
"babel-preset-forbeslindesay": "^2.1.0",
"babel-types": "^6.24.1",
"flow-bin": "^0.65.0",
"flow-bin": "^0.66.0",
"husky": "^0.14.3",
"jest": "^22.1.4",
"lint-staged": "^7.0.0",
Expand Down Expand Up @@ -66,4 +66,4 @@
"type": "git",
"url": "https://github.com/pugjs/babel-plugin-transform-react-pug.git"
}
}
}
25 changes: 25 additions & 0 deletions src/__tests__/__snapshots__/attributes-shorthand.test.js.snap
@@ -0,0 +1,25 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`JavaScript output: transformed source code 1`] = `
"// To prevent warnings in console from react
const test = 10;

module.exports = <div data-first={true} data-second={true} data-positive={true} data-negative={false} data-check={true}><div data-one={true} data-two={true} /></div>;"
`;

exports[`html output: generated html 1`] = `
<div
data-check={true}
data-first={true}
data-negative={false}
data-positive={true}
data-second={true}
>
<div
data-one={true}
data-two={true}
/>
</div>
`;

exports[`static html output: static html 1`] = `"<div data-first=\\"true\\" data-second=\\"true\\" data-positive=\\"true\\" data-negative=\\"false\\" data-check=\\"true\\"><div data-one=\\"true\\" data-two=\\"true\\"></div></div>"`;
13 changes: 13 additions & 0 deletions src/__tests__/attributes-shorthand.input.js
@@ -0,0 +1,13 @@
// To prevent warnings in console from react
const test = 10;

module.exports = pug`
div(
data-first
data-second
data-positive=true
data-negative=false
data-check
)
div(data-one data-two)
`;
3 changes: 3 additions & 0 deletions src/__tests__/attributes-shorthand.test.js
@@ -0,0 +1,3 @@
import testHelper from './test-helper';

testHelper(__dirname + '/attributes-shorthand.input.js');
56 changes: 56 additions & 0 deletions src/__tests__/attributes-unescaped.test.js
@@ -0,0 +1,56 @@
import React from 'react';
import {transform} from 'babel-core';
import renderer from 'react-test-renderer';
import transformReactPug from '../';

const transformationOptions = {
babelrc: false,
plugins: [transformReactPug],
};

const transformer = code => {
return transform(`pug\`${code}\``, transformationOptions).code;
};

const ExpectedError = /Unescaped attributes/;

test('throws error when pass string', () => {
const wrapped = () =>
transformer(`
div(name!="hello")
`);

expect(wrapped).toThrowError(ExpectedError);
});

test('throws error when pass variable', () => {
const wrapped = () =>
transformer(`
- const variable = 'value'
div(name!=variable.toString())
`);

expect(wrapped).toThrowError(ExpectedError);
});

test('does not throw error when pass variable or just string', () => {
const wrapped = () =>
transformer(`
- const variable = 'value'
div#id.class(
data-string="hello" data-variable=variable
)
div(class=['one', 'two'])
`);

expect(wrapped).not.toThrowError(ExpectedError);
});

test('does not throw error when pass boolean variables', () => {
const wrapped = () =>
transformer(`
div(data-first data-second data-third)
`);

expect(wrapped).not.toThrowError(ExpectedError);
});
15 changes: 10 additions & 5 deletions src/context.js
@@ -1,6 +1,6 @@
// @flow

import {readFileSync} from 'fs';
import {readFileSync, existsSync} from 'fs';
import error from 'pug-error';
import type {Key} from './block-key';
import {getCurrentLocation} from './babel-types';
Expand Down Expand Up @@ -42,12 +42,17 @@ class Context {
}

error(code: string, message: string): Error {
const src = readFileSync(this.file.opts.filename, 'utf8');
return error(code, message, {
const options: Object = {
filename: this.file.opts.filename,
line: getCurrentLocation().start.line - 1,
src,
});
src: null,
};

if (existsSync(options.filename)) {
options.src = readFileSync(this.file.opts.filename, 'utf8');
}

return error(code, message, options);
}

noKey<T>(fn: (context: Context) => T): T {
Expand Down
62 changes: 45 additions & 17 deletions src/visitors/Tag.js
Expand Up @@ -6,6 +6,14 @@ import t from '../babel-types';
import {visitJsx, visitJsxExpressions} from '../visitors';
import {getInterpolationRefs} from '../utils/interpolation';

type PugAttribute = {
name: string,
val: string,
mustEscape: boolean,
};

type Attribute = JSXAttribute | JSXSpreadAttribute;

/**
* Get children nodes from the node, passing the node's
* context to the children and generating JSX values.
Expand All @@ -27,15 +35,22 @@ function getChildren(node: Object, context: Context): Array<JSXValue> {
* them into JSX attributes.
* @param {Object} node - The node
* @param {Context} context - The context
* @returns {Array<JSXAttribute|JSXSpreadAttribute>}
* @returns {Array<Attribute>}
*/
function getAttributes(
node: Object,
context: Context,
): Array<JSXAttribute | JSXSpreadAttribute> {
const classes = [];
const attrs = node.attrs
.map(({name, val, mustEscape}) => {
function getAttributes(node: Object, context: Context): Array<Attribute> {
const classes: Array<Object> = [];
const attrs: Array<Attribute> = node.attrs
.map((node: PugAttribute): PugAttribute => {
if (node.val === true) {
return {
...node,
mustEscape: false,
};
}

return node;
})
.map(({name, val, mustEscape}: PugAttribute): Attribute | null => {
if (/\.\.\./.test(name) && val === true) {
return t.jSXSpreadAttribute(parseExpression(name.substr(3), context));
}
Expand All @@ -54,8 +69,19 @@ function getAttributes(

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

throw new Error('Unescaped attributes are not supported in react-pug');
if (!mustEscape) {
const isStringViaAliases: boolean =
Copy link
Member

Choose a reason for hiding this comment

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

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

t.isStringLiteral(expr) && !['className', 'id'].includes(name);
Copy link
Member

Choose a reason for hiding this comment

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

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


const isNotStringOrBoolean: boolean =
Copy link
Member

Choose a reason for hiding this comment

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

ditto

!t.isStringLiteral(expr) && !t.isBooleanLiteral(expr);
Copy link
Member

Choose a reason for hiding this comment

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

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) {

Copy link
Member Author

Choose a reason for hiding this comment

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

@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:

  1. if attribute is a string not from aliases (not from className and id)
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

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

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

  1. className and id if the value is a string literal
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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


if (isStringViaAliases || isNotStringOrBoolean) {
throw context.error(
'INVALID_EXPRESSION',
'Unescaped attributes are not supported in react-pug',
);
}
}

if (expr == null) {
Expand All @@ -79,20 +105,22 @@ function getAttributes(
return t.jSXAttribute(t.jSXIdentifier(name), jsxValue);
})
.filter(Boolean);

if (classes.length) {
const value = classes.every(cls => t.isStringLiteral(cls))
? t.stringLiteral(classes.map(cls => (cls: any).value).join(' '))
: t.jSXExpressionContainer(
t.callExpression(
t.memberExpression(
t.arrayExpression(classes),
t.identifier('join'),
t.callExpression(
t.memberExpression(
t.arrayExpression(classes),
t.identifier('join'),
),
[t.stringLiteral(' ')],
),
[t.stringLiteral(' ')],
),
);
);
attrs.push(t.jSXAttribute(t.jSXIdentifier('className'), value));
}

return attrs;
}

Expand Down