Fix for malformed output. - #9
Conversation
Without this change, you can get malformed stuff like this:
var foo = function() {
return "foo";
}var bar = function() {
return "bar";
}
By introducing a newline, a semicolon is implicitly inserted between statements
when necessary:
var foo = function() {
return "foo";
}
var bar = function() {
return "bar";
}
The indentation as a result of this commit is off, but I’m not sure how to fix
it because the build fails for me. (I tested this by editing dist directly.)
|
The build no longer fails for me (inexplicably), but the tests don’t pass. Hopefully you can figure out the right solution? 😁 |
|
FWIW, I found another bug, but don’t know what the solution is yet. It generated malformed output when I had a default export at the top of a file rather than at the bottom. The file that generated the error looked like this: // The leaf groups of the selection hierarchy are initially NodeList,
// and then lazily converted to arrays when mutation is required.
export default function(selection) {
return selection._root = arrayifyNode(selection._root, selection._depth);
};
function arrayifyNode(nodes, depth) {
var i = -1,
n = nodes.length,
node;
if (--depth) {
while (++i < n) {
if (node = nodes[i]) {
nodes[i] = arrayifyNode(node, depth);
}
}
}
else if (!Array.isArray(nodes)) {
var array = new Array(n);
while (++i < n) array[i] = nodes[i];
array._parent = nodes._parent;
nodes = array;
}
return nodes;
}And the fixed file that worked around the error: function arrayifyNode(nodes, depth) {
var i = -1,
n = nodes.length,
node;
if (--depth) {
while (++i < n) {
if (node = nodes[i]) {
nodes[i] = arrayifyNode(node, depth);
}
}
}
else if (!Array.isArray(nodes)) {
var array = new Array(n);
while (++i < n) array[i] = nodes[i];
array._parent = nodes._parent;
nodes = array;
}
return nodes;
}
// The leaf groups of the selection hierarchy are initially NodeList,
// and then lazily converted to arrays when mutation is required.
export default function(selection) {
return selection._root = arrayifyNode(selection._root, selection._depth);
};The malformed output was: function arrayifyNode(nodes, depth) {
var i = -1,
n = nodes.length,
node;
if (--depth) {
while (++i < n) {
if (node = nodes[i]) {
nodes[i] = arrayifyNode(node, depth);
}
}
}
else if (!Array.isArray(nodes)) {
var array = new Array(n);
while (++i < n) array[i] = nodes[i];
array._parent = nodes._parent;
nodes = array;
}
return nodes;
}
// The leaf groups of the selection hierarchy are initially NodeList,
// and then lazily converted to arrays when mutation is required.var arrayify = function(selection) {
return selection._root = arrayifyNode(selection._root, selection._depth);
}Edit: oh… you can see the |
YOU get a newline! Everyone gets a newline!
Use function(exports), not function('exports').
|
The non-deterministic test results might possibly be related to Rich-Harris/magic-string#12 (magic-string is basically a hack for manipulating strings in a sourcemap-aware way - saves a bunch on AST manipulation and code generation but there are some rough edges around indentation), will look into it. Any chance you still have the code that was generating the first malformed output? I can't seem to reproduce it, though I've definitely noticed it before in passing (albeit harmlessly up till now), and want to make sure we can guard against regressions. |
|
Sorry, I misunderstood you when you said the tests were failing - it's because they're expecting particular whitespace. The idea was to try and preserve the rhythm of the original code and keep comments attached to the right lines as far as possible, like this... // these statements are on consecutive lines
one();
two();
three();
thenWeHaveABreak();
/**
* JSDoc comment
*/
andAnotherBreak(); // trailing line comments are kept in place too...so padding each statement with newlines creates different output. I'm probably being too anal, though I might try an approach that simply guarantees at least one newline before each statement, and otherwise sticks to the existing method. That should fix the comment bug as well |
|
I’m all for preserving the original formatting of the code as much as possible. I think that’s a lovely feature. I was just bashing it with a hammer to get it to work and look forward to your more-correct fix. |
|
This is fixed and released in 0.7.0 |
|
Works on my end! Thank you. I did notice a few missing newlines still, but everything seemed to run okay. I only noticed it when there were comments (maybe comments at the start of a file?). For example, see the comments here: var emptyOf = function(selection) {
return new Selection(emptyNode(arrayify(selection), selection._depth), selection._depth);
}// Lazily constructs the exit selection for this (update) selection.
// Until this selection is joined to data, the exit selection will be empty.
var selection_exit = function() {
return this._exit || (this._exit = emptyOf(this));
}// Lazily constructs the enter selection for this (update) selection.
// Until this selection is joined to data, the enter selection will be empty.
var selection_enter = function() {
if (!this._enter) {
this._enter = emptyOf(this);
this._enter._update = this;
}
return this._enter;
}If you clone the d3-selection repo you should be able to see this in build/d3.js after running |
|
@mbostock comment handling is improved in 0.7.3 - I've tried it out with d3-selection and it behaves a bit better |
Without this change, you can get malformed stuff like this:
(Uncaught SyntaxError: Unexpected token var.)
By appending a newline, a semicolon is implicitly inserted between statements when necessary:
The indentation as a result of this commit is off, but I’m not sure how to fix it because the build fails for me. (I tested this by editing dist directly.)