From fe7ebc0cf750316b685fd79c92f32defe4a12989 Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Thu, 13 Apr 2017 09:21:18 -0700 Subject: [PATCH] Fix edge cases triggered by newlines in arrow functions (#1217) This one is pretty crazy. In #927, I changed ```js concat(["(", join(concat([", "]), printed), ")"]), ``` into ```js concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]), ``` which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks. In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly. The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway. The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around. I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1 Fix #1203 --- src/doc-utils.js | 11 +- .../__snapshots__/jsfmt.spec.js.snap | 150 ++++++++++++++++++ tests/last_argument_expansion/edge_case.js | 66 ++++++++ 3 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 tests/last_argument_expansion/edge_case.js diff --git a/src/doc-utils.js b/src/doc-utils.js index 61c5babe..f9ed9442 100644 --- a/src/doc-utils.js +++ b/src/doc-utils.js @@ -1,6 +1,6 @@ "use strict"; -function traverseDoc(doc, onEnter, onExit) { +function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) { var hasStopped = false; function traverseDocRec(doc) { if (onEnter) { @@ -21,6 +21,12 @@ function traverseDoc(doc, onEnter, onExit) { if (doc.flatContents) { traverseDocRec(doc.flatContents); } + } else if (doc.type === "group" && doc.expandedStates) { + if (shouldTraverseConditionalGroups) { + doc.expandedStates.forEach(traverseDocRec); + } else { + traverseDocRec(doc.contents); + } } else if (doc.contents) { traverseDocRec(doc.contents); } @@ -132,7 +138,8 @@ function propagateBreaks(doc) { breakParentGroup(groupStack); } } - } + }, + /* shouldTraverseConditionalGroups */ true ); } diff --git a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap index 905229cf..e66c325e 100644 --- a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap +++ b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap @@ -94,6 +94,156 @@ true `; +exports[`edge_case.js 1`] = ` +var listener = DOM.listen( + introCard, + 'click', + sigil, + (event: JavelinEvent): void => + BanzaiLogger.log( + config, + {...logData, ...DataStore.get(event.getNode(sigil))}, + ), +); + +a( + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong, + [ + { + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1 + } + ] +); + +exports.examples = [ + { + render: withGraphQLQuery( + 'node(1234567890){image{uri}}', + function(container, data) { + return ( +
+ + + +
+ ); + } + ) + } +]; + +someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([ + [], + // comment + [], +]); + +(function webpackUniversalModuleDefinition() {})(this, function(__WEBPACK_EXTERNAL_MODULE_85__, __WEBPACK_EXTERNAL_MODULE_115__) { +return /******/ (function(modules) { // webpackBootstrap + +/******/ }) +/************************************************************************/ +/******/ ([ +/* 0 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ }, +/* 1 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ }, +/* 2 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ } +/******/ ]) +}); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +var listener = DOM.listen( + introCard, + "click", + sigil, + (event: JavelinEvent): void => + BanzaiLogger.log(config, { + ...logData, + ...DataStore.get(event.getNode(sigil)) + }) +); + +a( + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong, + [ + { + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1 + } + ] +); + +exports.examples = [ + { + render: withGraphQLQuery( + "node(1234567890){image{uri}}", + function(container, data) { + return ( +
+ + + +
+ ); + } + ) + } +]; + +someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a( + [ + [], + // comment + [] + ] +); + +(function webpackUniversalModuleDefinition() {})( + this, + function(__WEBPACK_EXTERNAL_MODULE_85__, __WEBPACK_EXTERNAL_MODULE_115__) { + return /******/ (function(modules) { + // webpackBootstrap + /******/ + })( + /************************************************************************/ + /******/ [ + /* 0 */ + /***/ function(module, exports, __webpack_require__) { + /***/ + }, + /* 1 */ + /***/ function(module, exports, __webpack_require__) { + /***/ + }, + /* 2 */ + /***/ function(module, exports, __webpack_require__) { + /***/ + } + /******/ + ] + ); + } +); + +`; + exports[`jsx.js 1`] = ` const els = items.map(item => (
diff --git a/tests/last_argument_expansion/edge_case.js b/tests/last_argument_expansion/edge_case.js new file mode 100644 index 00000000..43373d5d --- /dev/null +++ b/tests/last_argument_expansion/edge_case.js @@ -0,0 +1,66 @@ +var listener = DOM.listen( + introCard, + 'click', + sigil, + (event: JavelinEvent): void => + BanzaiLogger.log( + config, + {...logData, ...DataStore.get(event.getNode(sigil))}, + ), +); + +a( + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong, + [ + { + SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1 + } + ] +); + +exports.examples = [ + { + render: withGraphQLQuery( + 'node(1234567890){image{uri}}', + function(container, data) { + return ( +
+ + + +
+ ); + } + ) + } +]; + +someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([ + [], + // comment + [], +]); + +(function webpackUniversalModuleDefinition() {})(this, function(__WEBPACK_EXTERNAL_MODULE_85__, __WEBPACK_EXTERNAL_MODULE_115__) { +return /******/ (function(modules) { // webpackBootstrap + +/******/ }) +/************************************************************************/ +/******/ ([ +/* 0 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ }, +/* 1 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ }, +/* 2 */ +/***/ function(module, exports, __webpack_require__) { + +/***/ } +/******/ ]) +});