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.

Sign up for GitHub

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

Refactor directive parsing for code reuse #1242

Merged
merged 1 commit into from
Mar 15, 2018
Merged

Refactor directive parsing for code reuse #1242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 102 additions & 98 deletions src/parse/read/directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,65 @@ import { parseExpressionAt } from 'acorn';
import repeat from '../../utils/repeat';
import { Parser } from '../index';

const DIRECTIVES = {
Ref: {
names: [ 'ref' ],
attribute(start, end, type, name) {
return { start, end, type, name };
}
},

EventHandler: {
names: [ 'on' ],
allowedExpressionTypes: [ 'CallExpression' ],
},

Binding: {
names: [ '', 'bind' ],
allowedExpressionTypes: [ 'Identifier', 'MemberExpression' ],
attribute(start, end, type, name, expression, directiveName) {
let value;

// :foo is shorthand for foo='{{foo}}'
if (!directiveName) {
const valueStart = start + 1;
const valueEnd = start + name.length;
type = 'Attribute';
value = getShorthandValue(start + 1, name);
} else {
value = expression || {
type: 'Identifier',
start: start + 5,
end,
name,
};
}

return { start, end, type, name, value };
},
},

Transition: {
names: [ 'in', 'out', 'transition' ],
allowedExpressionTypes: [ 'ObjectExpression' ],
attribute(start, end, type, name, expression, directiveName) {
return {
start, end, type, name, expression,
intro: directiveName === 'in' || directiveName === 'transition',
outro: directiveName === 'out' || directiveName === 'transition',
}
},
},
};


const lookupByName = {};

Object.keys(DIRECTIVES).forEach(name => {
const directive = DIRECTIVES[name];
directive.names.forEach(type => lookupByName[type] = name);
});

function readExpression(parser: Parser, start: number, quoteMark: string|null) {
let str = '';
let escaped = false;
Expand All @@ -24,7 +83,7 @@ function readExpression(parser: Parser, start: number, quoteMark: string|null) {
} else {
str += char;
}
} else if (/\s/.test(char)) {
} else if (/[\s\r\n\/>]/.test(char)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

\s should match the same character group as [\s\r\n]...

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#Special_characters_meaning_in_regular_expressions

Matches a single white space character, including space, tab, form feed, line feed and other Unicode spaces. Equivalent to [ \f\n\r\t\v\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff].

For example, /\s\w*/ matches " bar" in "foo bar".

Matches a single white space character, including space, tab, form feed, line feed and other Unicode spaces. Equivalent to [ \f\n\r\t\v\u00a0\u1680\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff].For example, /\s\w*/ matches " bar" in "foo bar".

Copy link
Member

Choose a reason for hiding this comment

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

ah whoops, missed this review before i hit merge, will account for these comments in a follow-up. sorry!

break;
} else {
str += char;
Expand All @@ -42,125 +101,70 @@ function readExpression(parser: Parser, start: number, quoteMark: string|null) {
return expression;
}

export function readEventHandlerDirective(
parser: Parser,
start: number,
name: string,
hasValue: boolean
) {
let expression;

if (hasValue) {
const quoteMark = parser.eat(`'`) ? `'` : parser.eat(`"`) ? `"` : null;

const expressionStart = parser.index;

expression = readExpression(parser, expressionStart, quoteMark);

if (expression.type !== 'CallExpression') {
parser.error(`Expected call expression`, expressionStart);
}
}

return {
start,
end: parser.index,
type: 'EventHandler',
name,
expression,
};
}

export function readBindingDirective(
export function readDirective(
parser: Parser,
start: number,
name: string
attrName: string
) {
let value;
const [ directiveName, name ] = attrName.split(':');
if (name === undefined) return; // No colon in the name

const type = lookupByName[directiveName];
if (!type) return; // not a registered directive

const directive = DIRECTIVES[type];
let expression = null;

if (parser.eat('=')) {
const quoteMark = parser.eat(`'`) ? `'` : parser.eat(`"`) ? `"` : null;

const a = parser.index;
const expressionStart = parser.index;

if (parser.eat('{{')) {
let message = 'bound values should not be wrapped';
const b = parser.template.indexOf('}}', a);
if (b !== -1) {
const value = parser.template.slice(parser.index, b);
let message = 'directive values should not be wrapped';
const expressionEnd = parser.template.indexOf('}}', expressionStart);
if (expressionEnd !== -1) {
const value = parser.template.slice(parser.index, expressionEnd);
message += ` — use '${value}', not '{{${value}}}'`;
}

parser.error(message, a);
parser.error(message, expressionStart);
}

// this is a bit of a hack so that we can give Acorn something parseable
let b;
if (quoteMark) {
b = parser.index = parser.template.indexOf(quoteMark, parser.index);
} else {
parser.readUntil(/[\s\r\n\/>]/);
b = parser.index;
}

const source = repeat(' ', a) + parser.template.slice(a, b);
value = parseExpressionAt(source, a, { ecmaVersion: 9 });

if (value.type !== 'Identifier' && value.type !== 'MemberExpression') {
parser.error(`Cannot bind to rvalue`, value.start);
expression = readExpression(parser, expressionStart, quoteMark);
if (directive.allowedExpressionTypes.indexOf(expression.type) === -1) {
parser.error(`Expected ${directive.allowedExpressionTypes.join(' or ')}`, expressionStart);
}
}

parser.allowWhitespace();

if (quoteMark) {
parser.eat(quoteMark, true);
}
if (directive.attribute) {
return directive.attribute(start, parser.index, type, name, expression, directiveName);
} else {
// shorthand – bind:foo equivalent to bind:foo='foo'
value = {
type: 'Identifier',
start: start + 5,
return {
start,
end: parser.index,
type: type,
name,
expression,
};
}

return {
start,
end: parser.index,
type: 'Binding',
name,
value,
};
}

export function readTransitionDirective(
parser: Parser,
start: number,
name: string,
type: string
) {
let expression = null;

if (parser.eat('=')) {
const quoteMark = parser.eat(`'`) ? `'` : parser.eat(`"`) ? `"` : null;

const expressionStart = parser.index;

expression = readExpression(parser, expressionStart, quoteMark);

if (expression.type !== 'ObjectExpression') {
parser.error(`Expected object expression`, expressionStart);
}
}

return {
start,
end: parser.index,
type: 'Transition',
name,
intro: type === 'in' || type === 'transition',
outro: type === 'out' || type === 'transition',
expression,
};
function getShorthandValue(start: number, name: string) {
const end = start + name.length;

return [
{
type: 'AttributeShorthand',
start,
end,
expression: {
type: 'Identifier',
start,
end,
name,
},
},
];
}
62 changes: 4 additions & 58 deletions src/parse/state/tag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import readExpression from '../read/expression';
import readScript from '../read/script';
import readStyle from '../read/style';
import {
readEventHandlerDirective,
readBindingDirective,
readTransitionDirective,
} from '../read/directives';
import { readDirective } from '../read/directives';
import { trimStart, trimEnd } from '../../utils/trim';
import { decodeCharacterReferences } from '../utils/html';
import isVoidElementName from '../../utils/isVoidElementName';
Expand Down Expand Up @@ -303,42 +299,10 @@ function readAttribute(parser: Parser, uniqueNames: Set<string>) {

parser.allowWhitespace();

if (/^on:/.test(name)) {
return readEventHandlerDirective(parser, start, name.slice(3), parser.eat('='));
}

if (/^bind:/.test(name)) {
return readBindingDirective(parser, start, name.slice(5));
}
const attribute = readDirective(parser, start, name);
if (attribute) return attribute;

if (/^ref:/.test(name)) {
return {
start,
end: parser.index,
type: 'Ref',
name: name.slice(4),
};
}

const match = /^(in|out|transition):/.exec(name);
if (match) {
return readTransitionDirective(
parser,
start,
name.slice(match[0].length),
match[1]
);
}

let value;

// :foo is shorthand for foo='{{foo}}'
if (/^:\w+$/.test(name)) {
name = name.slice(1);
value = getShorthandValue(start + 1, name);
} else {
value = parser.eat('=') ? readAttributeValue(parser) : true;
}
let value = parser.eat('=') ? readAttributeValue(parser) : true;

return {
start,
Expand All @@ -364,24 +328,6 @@ function readAttributeValue(parser: Parser) {
return value;
}

function getShorthandValue(start: number, name: string) {
const end = start + name.length;

return [
{
type: 'AttributeShorthand',
start,
end,
expression: {
type: 'Identifier',
start,
end,
name,
},
},
];
}

function readSequence(parser: Parser, done: () => boolean) {
let currentChunk: Node = {
start: parser.index,
Expand Down
2 changes: 1 addition & 1 deletion test/parser/samples/error-binding-mustaches/error.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"message": "bound values should not be wrapped — use 'foo', not '{{foo}}'",
"message": "directive values should not be wrapped — use 'foo', not '{{foo}}'",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this tests the same thing. A directive is not the same thing as a bound value...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Directives are special attributes with a colon. Bindings happen to be one type of directive, but all directive expressions should not be wrapped. This is a more general message for any directives. I could change it to use the specific name of the directive. E.g. Binding values should not be wrapped and EventHandler values should not be wrapped. But since the error points to the code where the issue is, I don't feel this provides much.

Copy link
Member

Choose a reason for hiding this comment

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

Can see both sides of this, though I think it's probably ok to leave in —

on:click='{{set({foo: 'bar' })}}'

is just as incorrect as bind:value='{{foo}}', it's just less likely that someone would make that mistake.

"loc": {
"line": 1,
"column": 19
Expand Down
2 changes: 1 addition & 1 deletion test/parser/samples/error-binding-rvalue/error.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"message": "Cannot bind to rvalue",
"message": "Expected Identifier or MemberExpression",
"pos": 19,
"loc": {
"line": 1,
Expand Down
2 changes: 1 addition & 1 deletion test/parser/samples/error-event-handler/error.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"message": "Expected call expression",
"message": "Expected CallExpression",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is less expressive than using English.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it is more literal. Are you thinking we should write a method to humanize the expression type in the errors?

"loc": {
"line": 1,
"column": 15
Expand Down