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.

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

Respect useTabs option when indenting an opening JSX element #404

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jsnajdr
Copy link

@jsnajdr jsnajdr commented May 10, 2017

Steps to reproduce:
Run this node script:

const recast = require('.');

// parse some code into AST
let ast = recast.parse([
	"function x(a, b) {",
	"\treturn (",
	"\t\t<div>",
	"\t\t\t{ b }",
	"\t\t</div>",
	"\t);",
	"}"
].join("\n"));

// change the variable identifier in JSX to trigger reprint
ast.program.body[0].body.body[0].argument.children[1].expression.name = 'a';

// print the output with useTabs=true
let output = recast.print(ast, { useTabs: true }).code;

// replace tabs with '~' and spaces with '_'
output = output.replace(/ /g, "_").replace(/\t/g, "~");

// print the output code with visible whitespace
console.log(output);

Expected result:
The output is indented with tabs:

function_x(a,_b)_{
~return_(
~~<div>
~~~{_a_}
~~</div>
~);
}

Actual result:
There are spaces before the JSX opening element:

function_x(a,_b)_{
~return_(
~____<div>
~~~{_a_}
~~</div>
~);
}

The cause of the bug is:

The fix is to use a smarter algorithm from the Lines.prototype.sliceString function.

However, this requires passing a new parameter to the join method and recursively add options to many many calls. I'm wondering if there is a better solution for this. Do all the calls to concat and Lines.prototype.join need the options argument?

lib/lines.js Outdated
result += new Array(spaces + 1).join(" ");
}

return result;
Copy link
Owner

Choose a reason for hiding this comment

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

Can you stick to four spaces per tab when adding new implementation code, please? I realize you're a fan of tabs (nothing wrong with that), but consistency is important here.

Copy link

@Hexcede Hexcede Oct 8, 2022

Choose a reason for hiding this comment

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

Can you stick to four spaces per tab when adding new implementation code, please? I realize you're a fan of tabs (nothing wrong with that), but consistency is important here.

I apologize in advance, but I really can't help emphasizing the enormous amount of irony in the statement "consistency is important here" in reference to indents in a PR that is targeting a design flaw that produces inconsistent indents. (Not trying to be snippy or anything, I'm sorry if it came across that way)

Could issue #315 as well as this PR possibly be addressed sometime soon since this project appears like it's still maintained? These issues have been around for almost six years now, it would be pretty nice. In the meantime I switched to babel's generator which seems to have everything I need.

@jsnajdr jsnajdr force-pushed the fix-lines-join-indent branch from 3043a97 to 02cfced Compare May 10, 2017 19:40
@jsnajdr
Copy link
Author

jsnajdr commented May 10, 2017

Can you stick to four spaces per tab when adding new implementation code, please? I realize you're a fan of tabs (nothing wrong with that), but consistency is important here.

Oh, sorry for submitting broken commits like this :( I pushed a tabs-to-spaces change, and also changed the indentString function to avoid param destructuring -- the tests were failing on older versions of Node.js that don't have good enough ES6 support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants