vue/define-macros-order bug #1861
Conversation
| @@ -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) && | |||
There was a problem hiding this comment.
Script setup stuff fixes multiple scripts bug(test is here: https://github.com/vuejs/eslint-plugin-vue/pull/1861/files#diff-6ebdd3c21be70e29ce94a2f399884fd220d4fe9bba358ade66b0bceb325d3102R387)
lib/rules/define-macros-order.js
Outdated
| @@ -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.
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.
Now we take space after the target token and added it after the moving token.
There was a problem hiding this comment.
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?
lib/rules/define-macros-order.js
Outdated
| afterTargetComment.range[0] | ||
| ) | ||
| // handle case when a();b() -> b()a(); | ||
| const invalidResult = !textNode.endsWith(';') && !/[\n;]/.test(afterText) |
There was a problem hiding this comment.
This is the corner case I found
|
I ran it in my project and the result was great. The only annoying thing left is this: There is an option to introduce "newLineAfterDefineMacros: true" option maybe... For now, I've done nothing |
lib/rules/define-macros-order.js
Outdated
| afterTargetComment.range[0] | ||
| ) | ||
| // handle case when a();b() -> b()a(); | ||
| const invalidResult = !textNode.endsWith(';') && !/[\n;]/.test(afterText) |
There was a problem hiding this comment.
I guess it should check the start only, right?
| const invalidResult = !textNode.endsWith(';') && !/[\n;]/.test(afterText) | |
| const invalidResult = !textNode.endsWith(';') && !/^[\n;]/.test(afterText) |
There was a problem hiding this comment.
No, this value also get valid result
const afterText = ' ; '
//a() ; b();There was a problem hiding this comment.
But wouldn't it also return true for b();c()? Or (b();)?
There was a problem hiding this comment.
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.
maybe i need to remove ; from regexp
There was a problem hiding this comment.
lib/rules/define-macros-order.js
Outdated
| @@ -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.
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?
|
@ota-meshi sure. Am I need to open an issue? |
|
If you write an explanation in the PR, you do not need to open an issue. |
Fix bugs from #1856