-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Improved parsing of Trans component #85
Changes from 6 commits
4302b1a
4bffe6a
00efb6d
23cd30d
a85d51c
c69892b
05f7f53
b18e60a
4038f6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
import * as acorn from 'acorn-jsx'; | ||
import assert from 'assert' | ||
import HTMLLexer from './html-lexer' | ||
import BaseLexer from './base-lexer'; | ||
import { ParsingError } from '../helpers'; | ||
|
||
export default class JsxLexer extends HTMLLexer { | ||
constructor(options = {}) { | ||
|
@@ -44,7 +48,7 @@ export default class JsxLexer extends HTMLLexer { | |
const key = attrs.keys | ||
|
||
if (matches[3] && !attrs.options.defaultValue) { | ||
attrs.options.defaultValue = matches[3].trim() | ||
attrs.options.defaultValue = this.eraseTags(matches[0]).replace(/\s+/g, ' ') | ||
} | ||
|
||
if (key) { | ||
|
@@ -54,4 +58,59 @@ export default class JsxLexer extends HTMLLexer { | |
|
||
return this.keys | ||
} | ||
|
||
/** | ||
* Recursively convert html tags and js injections to tags with the child index in it | ||
* | ||
* @param {string} string | ||
* | ||
* @returns string | ||
*/ | ||
eraseTags(string) { | ||
const children = this.simplify(acorn.parse(string, {plugins: {jsx: true}}).body[0].expression.children, string); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please avoid line that are longer than 80 char. I would suggest doing this in two steps |
||
|
||
const elemsToString = children => children.map((child, index) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you create a function rather than directly returning the mapped children? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it calls itself recurisvely. |
||
switch(child.type) { | ||
case 'text': return child.content; | ||
case 'js': return `<${index}>${child.content}</${index}>`; | ||
case 'tag': return `<${index}>${elemsToString(child.children)}</${index}>`; | ||
default: throw new ParsingError('Unknown parsed content: ' + child.type); | ||
} | ||
}).join(''); | ||
|
||
return elemsToString(children); | ||
} | ||
|
||
/** | ||
* Simplify the bulky AST given by Acorn | ||
* @param {*} children An array of elements contained inside an html tag | ||
* @param {string} originalString The original string being parsed | ||
*/ | ||
simplify(children, originalString) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you name this something a little more idiomatic like |
||
for (let i = 0; i < children.length; i ++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you don't use |
||
const child = children[i]; | ||
switch (child.type) { | ||
case 'JSXText': children[i] = { | ||
type: 'text', | ||
content: child.value.replace(/^(?:\s*(\n|\r)\s*)?(.*)(?:\s*(\n|\r)\s*)?$/, '$2') | ||
}; break | ||
case 'JSXElement': children[i] = { | ||
type: 'tag', | ||
children: this.simplify(child.children, originalString) | ||
}; break; | ||
case 'JSXExpressionContainer': children[i] = { | ||
type: 'js', | ||
content: originalString.slice(child.start, child.end) | ||
}; break; | ||
default: throw new ParsingError("Unknown ast element when parsing jsx: " + child.type) | ||
} | ||
} | ||
|
||
// Filter empty text elements. Using string.length instead of !string because | ||
// '0' is a valid text element, and '' is not, and !string doesn't make a difference | ||
// between the two. | ||
children = children.filter(child => !(child.type === 'text' && child.content.length === 0)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be written I'm not sure about your comment either. As I commented above: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, |
||
|
||
return children; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please test these methods in the lexer's test? The parser test is a high level check but individual method that do as much as these one should have test of their own so we can debug them later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test for
eraseTags
.