250 precursor #293
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| node_modules/ | ||
| npm-debug.log | ||
| .DS_Store | ||
| latest-change.txt | ||
| patternlab.json | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,110 +13,227 @@ | |
| var parameter_hunter = function () { | ||
|
|
||
| var extend = require('util')._extend, | ||
| JSON = require('json5'), | ||
|
There was a problem hiding this comment. Same comment as above. I'd consider renaming to |
||
| pa = require('./pattern_assembler'), | ||
| smh = require('./style_modifier_hunter'), | ||
| pattern_assembler = new pa(), | ||
| style_modifier_hunter = new smh(); | ||
|
|
||
| /** | ||
| * This function is really to accommodate the lax JSON-like syntax allowed by | ||
| * Pattern Lab PHP for parameter submissions to partials. Unfortunately, no | ||
| * easily searchable library was discovered for this. What we had to do was | ||
| * write a custom script to crawl through the parameter string, and wrap the | ||
| * keys and values in double-quotes as necessary. | ||
| * The steps on a high-level are as follows: | ||
| * * Further escape all escaped quotes, commas, and colons. Use the string | ||
| * representation of their unicodes for this. This has the added bonus | ||
| * of being interpreted correctly by JSON5.parse() without further | ||
| * modification. This will be useful later in the function. | ||
| * * Once escaped quotes are out of the way, we know the remaining quotes | ||
| * are either key/value wrappers or wrapped within those wrappers. We know | ||
| * that remaining commas and colons are either delimiters, or wrapped | ||
| * within quotes to not be recognized as such. | ||
| * * A do-while loop crawls paramString to write keys to a keys array and | ||
| * values to a values array. | ||
| * * Start by parsing the first key. Determine the type of wrapping quote, | ||
| * if any. | ||
| * * By knowing the open wrapper, we know that the next quote of that kind | ||
| * (if the key is wrapped in quotes), HAS to be the close wrapper. | ||
| * Similarly, if the key is unwrapped, we know the next colon HAS to be | ||
| * the delimiter between key and value. | ||
| * * Save the key to the keys array. | ||
| * * Next, search for a value. It will either be the next block wrapped in | ||
| * quotes, or a string of alphanumerics, decimal points, or minus signs. | ||
| * * Save the value to the values array. | ||
| * * The do-while loop truncates the paramString value while parsing. Its | ||
| * condition for completion is when the paramString is whittled down to an | ||
| * empty string. | ||
| * * After the keys and values arrays are built, a for loop iterates through | ||
| * them to build the final paramStringWellFormed string. | ||
| * * No quote substitution had been done prior to this loop. In this loop, | ||
| * all keys are ensured to be wrapped in double-quotes. String values are | ||
| * also ensured to be wrapped in double-quotes. | ||
| * * Unescape escaped unicodes except for double-quotes. Everything beside | ||
| * double-quotes will be wrapped in double-quotes without need for escape. | ||
| * * Return paramStringWellFormed. | ||
| * | ||
| * @param {string} pString | ||
| * @returns {string} paramStringWellFormed | ||
| */ | ||
| function paramToJson(pString) { | ||
| var colonPos = -1; | ||
| var keys = []; | ||
| var paramString = pString; // to not reassign param | ||
| var paramStringWellFormed; | ||
| var quotePos = -1; | ||
| var regex; | ||
| var values = []; | ||
| var wrapper; | ||
|
|
||
| //replace all escaped double-quotes with escaped unicode | ||
| paramString = paramString.replace(/\\"/g, '\\u0022'); | ||
|
|
||
| //replace all escaped single-quotes with escaped unicode | ||
| paramString = paramString.replace(/\\'/g, '\\u0027'); | ||
|
|
||
| //replace all escaped commas with escaped unicode | ||
| paramString = paramString.replace(/\\,/g, '\\u0044'); | ||
|
|
||
| //replace all escaped colons with escaped unicode | ||
| paramString = paramString.replace(/\\:/g, '\\u0058'); | ||
|
|
||
| //with escaped chars out of the way, crawl through paramString looking for | ||
| //keys and values | ||
| do { | ||
|
|
||
| //check if searching for a key | ||
| if (paramString[0] === '{' || paramString[0] === ',') { | ||
| paramString = paramString.substring([1], paramString.length).trim(); | ||
|
There was a problem hiding this comment. The first There was a problem hiding this comment. @james-nash Great catch! There was a problem hiding this comment. I found another instance of using an array instead of a number for |
||
|
|
||
| //search for end quote if wrapped in quotes. else search for colon. | ||
| //everything up to that position will be saved in the keys array. | ||
| switch (paramString[0]) { | ||
|
|
||
| //need to search for end quote pos in case the quotes wrap a colon | ||
| case '"': | ||
| case '\'': | ||
| wrapper = paramString[0]; | ||
| quotePos = paramString.indexOf(wrapper, 1); | ||
| break; | ||
|
|
||
| default: | ||
| colonPos = paramString.indexOf(':'); | ||
| } | ||
|
|
||
| if (quotePos > -1) { | ||
| keys.push(paramString.substring(0, quotePos + 1).trim()); | ||
|
|
||
| //truncate the beginning from paramString and look for a value | ||
| paramString = paramString.substring(quotePos + 1, paramString.length).trim(); | ||
|
|
||
| //unset quotePos | ||
| quotePos = -1; | ||
|
|
||
| } else if (colonPos > -1) { | ||
| keys.push(paramString.substring(0, colonPos).trim()); | ||
|
|
||
| //truncate the beginning from paramString and look for a value | ||
| paramString = paramString.substring(colonPos, paramString.length); | ||
|
|
||
| //unset colonPos | ||
| colonPos = -1; | ||
|
|
||
| //if there are no more colons, and we're looking for a key, there is | ||
| //probably a problem. stop any further processing. | ||
| } else { | ||
| paramString = ''; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| //now, search for a value | ||
| if (paramString[0] === ':') { | ||
| paramString = paramString.substring([1], paramString.length).trim(); | ||
|
|
||
| //since a quote of same type as its wrappers would be escaped, and we | ||
| //escaped those even further with their unicode, it is safe to look for | ||
| //wrapper pairs and conclude that their contents are values | ||
| switch (paramString[0]) { | ||
| case '"': | ||
| regex = /^"(.|\s)*?"/; | ||
|
There was a problem hiding this comment. This regex can be simplified to There was a problem hiding this comment. @james-nash There was a problem hiding this comment. @e2tha-e Ah! OK, I missed that. Ignore that comment then. |
||
| break; | ||
| case '\'': | ||
| regex = /^'(.|\s)*?'/; | ||
| break; | ||
|
|
||
| //if there is no value wrapper, regex for alphanumerics, decimal | ||
| //points, and minus signs for exponential notation. | ||
| default: | ||
| regex = /^[\w\-\.]*/; | ||
| } | ||
| values.push(paramString.match(regex)[0].trim()); | ||
|
|
||
| //truncate the beginning from paramString and continue either | ||
| //looking for a key, or returning | ||
| paramString = paramString.replace(regex, '').trim(); | ||
|
|
||
| //exit do while if the final char is '}' | ||
| if (paramString === '}') { | ||
| paramString = ''; | ||
| break; | ||
| } | ||
|
|
||
| //if there are no more colons, and we're looking for a value, there is | ||
| //probably a problem. stop any further processing. | ||
| } else { | ||
| paramString = ''; | ||
| break; | ||
| } | ||
| } while (paramString); | ||
|
|
||
| //build paramStringWellFormed string for JSON parsing | ||
| paramStringWellFormed = '{'; | ||
| for (var i = 0; i < keys.length; i++) { | ||
|
|
||
| //keys | ||
| //replace single-quote wrappers with double-quotes | ||
| if (keys[i][0] === '\'' && keys[i][keys[i].length - 1] === '\'') { | ||
| paramStringWellFormed += '"'; | ||
|
|
||
| //any enclosed double-quotes must be escaped | ||
| paramStringWellFormed += keys[i].substring(1, keys[i].length - 1).replace(/"/g, '\\"'); | ||
| paramStringWellFormed += '"'; | ||
| } else { | ||
|
|
||
| //open wrap with double-quotes if no wrapper | ||
| if (keys[i][0] !== '"' && keys[i][0] !== '\'') { | ||
| paramStringWellFormed += '"'; | ||
|
|
||
| //this is to clean up vestiges from Pattern Lab PHP's escaping scheme. | ||
| //F.Y.I. Pattern Lab PHP would allow special characters like question | ||
| //marks in parameter keys so long as the key was unwrapped and the | ||
| //special character escaped with a backslash. In Node, we need to wrap | ||
| //those keys and unescape those characters. | ||
| keys[i] = keys[i].replace(/\\/g, ''); | ||
| } | ||
|
|
||
| paramStringWellFormed += keys[i]; | ||
|
|
||
| //close wrap with double-quotes if no wrapper | ||
| if (keys[i][keys[i].length - 1] !== '"' && keys[i][keys[i].length - 1] !== '\'') { | ||
| paramStringWellFormed += '"'; | ||
| } | ||
| } | ||
|
|
||
| //colon delimiter. | ||
| paramStringWellFormed += ':'; + values[i]; | ||
|
|
||
| //values | ||
| //replace single-quote wrappers with double-quotes | ||
| if (values[i][0] === '\'' && values[i][values[i].length - 1] === '\'') { | ||
| paramStringWellFormed += '"'; | ||
|
|
||
| //any enclosed double-quotes must be escaped | ||
| paramStringWellFormed += values[i].substring(1, values[i].length - 1).replace(/"/g, '\\"'); | ||
| paramStringWellFormed += '"'; | ||
|
|
||
| //for everything else, just add the value however it's wrapped | ||
| } else { | ||
| paramStringWellFormed += values[i]; | ||
| } | ||
|
|
||
| //comma delimiter | ||
| if (i < keys.length - 1) { | ||
| paramStringWellFormed += ','; | ||
| } | ||
| } | ||
| paramStringWellFormed += '}'; | ||
|
|
||
| //unescape escaped unicode except for double-quotes | ||
| paramStringWellFormed = paramStringWellFormed.replace(/\\u0027/g, '\''); | ||
| paramStringWellFormed = paramStringWellFormed.replace(/\\u0044/g, ','); | ||
| paramStringWellFormed = paramStringWellFormed.replace(/\\u0058/g, ':'); | ||
|
|
||
| return paramStringWellFormed; | ||
| } | ||
|
|
@@ -140,7 +257,7 @@ var parameter_hunter = function () { | |
|
|
||
| //strip out the additional data, convert string to JSON. | ||
| var leftParen = pMatch.indexOf('('); | ||
| var rightParen = pMatch.lastIndexOf(')'); | ||
| var paramString = '{' + pMatch.substring(leftParen + 1, rightParen) + '}'; | ||
| var paramStringWellFormed = paramToJson(paramString); | ||
|
|
||
|
|
@@ -152,8 +269,9 @@ var parameter_hunter = function () { | |
| paramData = JSON.parse(paramStringWellFormed); | ||
| globalData = JSON.parse(JSON.stringify(patternlab.data)); | ||
| localData = JSON.parse(JSON.stringify(pattern.jsonFileData || {})); | ||
| } catch (err) { | ||
| console.log('There was an error parsing JSON for ' + pattern.abspath); | ||
| console.log(err); | ||
| } | ||
|
|
||
| var allData = pattern_assembler.merge_data(globalData, localData); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wonder if it might be more appropriate to name this
JSON5, so that people don't mistake it for the built-inJSONobject.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.
@james-nash Had to debate over this myself, and I'm still torn. @bmuenzenmeyer What are your thoughts?
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'm not too fussed myself. However, if you were to change it to
JSON5then I'd suggest doing that throughout all files where you use that library.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.
Forgive my naivety on JSON5 (it looks like a nice solution to a problem we've all faced), but it doesn't sit right with me. My biggest worry is downstream consumption of .json5 files (I'd also argue that things like trailing commas are not good form. Yes comments would be nice). If people get accustomed to writing these files, or perhaps worse, writing them as "invalid" pure .json files, they might shoot themselves or others in the foot. Is this fear overblown?
Another consideration in my rather scattershot comment is that it looks like they don't follow semver (yet), which could create problems.
... more thinking ...
Put simply, can you explain how adding JSON5 makes #250 (which PR references by name) and the parameter_hunter.paramToJson() method better?
I am an outsider on the specifics of this issue, and have no doubt that a lot of thought has been put into getting us this far, but I need your help in understanding if this is straying too far from the goal of this issue/fix. c8acb5a mentions better error handling, but in what way is it better than the existing try catch logic? http://codepen.io/bmuenzenmeyer/pen/66eb18de343ab1954abc53537fde09f0 has an example that is descriptive enough, coupled with a bit more logging on PL's part, would seem to do the trick in my mind.
Thoughts?
cc @geoffp
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.
It's important to note that the pattern parameter syntax is only vaguely defined here http://patternlab.io/docs/pattern-parameters.html#pattern-parameter-syntax , but since the examples have keys that are not double-quoted, it's definitely not pure JSON. It's JSON-ish :-)
I'd be in favour of keeping our parsing of the pattern parameter syntax quite lax, since it's conceivable that users might copy-paste chunks of JSON data (so then keys might be double-quoted) and, as per issue #145 it's desirable to allow HTML code to passed in as a value so allowing single- as well as double-quoted string values makes sense to me (saves you having to escape every quote around HTML attributes and stuff like that).
The changes in this PR which make use of JSON5 enable all of that, so I think they're worth keeping.
Since, I've been focussed primarily on the parsing of pattern parameters, which is in
parameter_hunter.paramToJson(), I'm not as familiar with the other code though. Perhaps @e2tha-e can comment on that.FWIW, where proper JSON data is expected, I'd agree that it might be better to be stricter (or at least display warnings in the build output) so as not to encourage sloppy JSON code that may not be interoperable with other tools or PatternLab versions.