-
-
Notifications
You must be signed in to change notification settings - Fork 681
vue/define-macros-order bug #1861
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,6 +19,17 @@ const MACROS_PROPS = 'defineProps' | |||||
const ORDER = [MACROS_EMITS, MACROS_PROPS] | ||||||
const DEFAULT_ORDER = [MACROS_PROPS, MACROS_EMITS] | ||||||
|
||||||
/** | ||||||
* @param {VElement} scriptSetup | ||||||
* @param {ASTNode} node | ||||||
*/ | ||||||
function inScriptSetup(scriptSetup, node) { | ||||||
return ( | ||||||
scriptSetup.range[0] <= node.range[0] && | ||||||
node.range[1] <= scriptSetup.range[1] | ||||||
) | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {ASTNode} node | ||||||
*/ | ||||||
|
@@ -33,9 +44,10 @@ function isUseStrictStatement(node) { | |||||
/** | ||||||
* Get an index of the first statement after imports and interfaces in order | ||||||
* to place defineEmits and defineProps before this statement | ||||||
* @param {VElement} scriptSetup | ||||||
* @param {Program} program | ||||||
*/ | ||||||
function getTargetStatementPosition(program) { | ||||||
function getTargetStatementPosition(scriptSetup, program) { | ||||||
const skipStatements = new Set([ | ||||||
'ImportDeclaration', | ||||||
'TSInterfaceDeclaration', | ||||||
|
@@ -45,7 +57,11 @@ function getTargetStatementPosition(program) { | |||||
]) | ||||||
|
||||||
for (const [index, item] of program.body.entries()) { | ||||||
if (!skipStatements.has(item.type) && !isUseStrictStatement(item)) { | ||||||
if ( | ||||||
inScriptSetup(scriptSetup, item) && | ||||||
!skipStatements.has(item.type) && | ||||||
!isUseStrictStatement(item) | ||||||
) { | ||||||
return index | ||||||
} | ||||||
} | ||||||
|
@@ -104,7 +120,10 @@ function create(context) { | |||||
'Program:exit'(program) { | ||||||
const shouldFirstNode = macrosNodes.get(order[0]) | ||||||
const shouldSecondNode = macrosNodes.get(order[1]) | ||||||
const firstStatementIndex = getTargetStatementPosition(program) | ||||||
const firstStatementIndex = getTargetStatementPosition( | ||||||
scriptSetup, | ||||||
program | ||||||
) | ||||||
const firstStatement = program.body[firstStatementIndex] | ||||||
|
||||||
// have both defineEmits and defineProps | ||||||
|
@@ -213,13 +232,12 @@ function create(context) { | |||||
const targetComment = sourceCode.getTokenAfter(beforeTargetToken, { | ||||||
includeComments: true | ||||||
}) | ||||||
const textSpace = getTextBetweenTokens(beforeTargetToken, targetComment) | ||||||
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 thing fixes these newlines https://github.com/vuejs/eslint-plugin-vue/pull/1861/files#diff-6ebdd3c21be70e29ce94a2f399884fd220d4fe9bba358ade66b0bceb325d3102L158 Before we took space before the target token and added it after the moving token. 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. Hmmm. I think this change is an improvement. I don't think this should be included in the patch, so could you revert this change in this PR and open another PR? |
||||||
// make insert text: comments + node + space before target | ||||||
const textNode = sourceCode.getText( | ||||||
node, | ||||||
node.range[0] - nodeComment.range[0] | ||||||
) | ||||||
const insertText = textNode + textSpace | ||||||
const insertText = getInsertText(textNode, target) | ||||||
|
||||||
return [ | ||||||
fixer.insertTextBefore(targetComment, insertText), | ||||||
|
@@ -228,17 +246,28 @@ function create(context) { | |||||
} | ||||||
|
||||||
/** | ||||||
* @param {ASTNode} tokenBefore | ||||||
* @param {ASTNode} tokenAfter | ||||||
* Get result text to insert | ||||||
* @param {string} textNode | ||||||
* @param {ASTNode} target | ||||||
*/ | ||||||
function getTextBetweenTokens(tokenBefore, tokenAfter) { | ||||||
return sourceCode.text.slice(tokenBefore.range[1], tokenAfter.range[0]) | ||||||
function getInsertText(textNode, target) { | ||||||
const afterTargetComment = sourceCode.getTokenAfter(target, { | ||||||
includeComments: true | ||||||
}) | ||||||
const afterText = sourceCode.text.slice( | ||||||
target.range[1], | ||||||
afterTargetComment.range[0] | ||||||
) | ||||||
// handle case when a();b() -> b()a(); | ||||||
const invalidResult = !textNode.endsWith(';') && !/[\n;]/.test(afterText) | ||||||
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 is the corner case I found 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. I guess it should check the start only, right?
Suggested change
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. No, this value also get valid result const afterText = ' ; '
//a() ; b(); 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. But wouldn't it also return true for 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. hm. This variable contains only whitespaces and new lines I suppose... because it's all can exist between node and next comment after node 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. maybe i need to remove 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. |
||||||
|
||||||
return textNode + afterText + (invalidResult ? ';' : '') | ||||||
} | ||||||
|
||||||
/** | ||||||
* Get position of the beginning of the token's line(or prevToken end if no line) | ||||||
* @param {ASTNode} token | ||||||
* @param {ASTNode} prevToken | ||||||
* @param {ASTNode|Token} token | ||||||
* @param {ASTNode|Token} prevToken | ||||||
*/ | ||||||
function getLineStartIndex(token, prevToken) { | ||||||
// if we have next token on the same line - get index right before that token | ||||||
|
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.
Script setup stuff fixes multiple scripts bug(test is here: https://github.com/vuejs/eslint-plugin-vue/pull/1861/files#diff-6ebdd3c21be70e29ce94a2f399884fd220d4fe9bba358ade66b0bceb325d3102R387)