Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make xstate-layout string placement more robust to changes #287

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Make xstate-layout string placement more robust to changes #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9bc3916
Fix multiple layout issue
davidkpiano Jan 8, 2023
d41a449
Fix multiple layout issue
davidkpiano Jan 8, 2023
c7cccf7
Oops
davidkpiano Jan 8, 2023
49daa5c
Add tests
davidkpiano Jan 8, 2023
d572bc5
Add back layout string
davidkpiano Jan 8, 2023
052e14d
Remove script
davidkpiano Jan 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/machine-extractor/src/MachineExtractResult.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import * as recast from 'recast';
import { Action, Condition, MachineOptions } from 'xstate';
import { choose } from 'xstate/lib/actions';
import { DeclarationType } from '.';
import { RecordOfArrays } from './RecordOfArrays';
import { ActionNode, ParsedChooseCondition } from './actions';
import { getMachineNodesFromFile } from './getMachineNodesFromFile';
import { TMachineCallExpression } from './machineCallExpression';
import { RecordOfArrays } from './RecordOfArrays';
import { StateNodeReturn } from './stateNode';
import { toMachineConfig, ToMachineConfigOptions } from './toMachineConfig';
import { ToMachineConfigOptions, toMachineConfig } from './toMachineConfig';
import { TransitionConfigNode } from './transitions';
import { Comment } from './types';

Expand Down Expand Up @@ -338,10 +338,10 @@ export class MachineExtractResult {
return false;
}

return (
comment.loc!.end.line ===
this.machineCallResult.callee.loc!.start.line - 1
);
const proximity =
this.machineCallResult.callee.loc!.start.line - comment.loc!.end.line;

return Math.abs(proximity) <= 2;
Comment on lines +341 to +344
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure why we test this._fileAst.comments in the first place. Perhaps it's to "simplify" the logic to locate it when it's put above the variable declaration. I don't think this is the best solution here though.

We should rely more on .leadingComments - we don't have to loop through all existing comments in the file if we can rely on more local ones. And when we take a look at how .leadingComments behave then we can notice that they can actually be far away from the node, as long as there are no AST nodes in between the comment and the node. Based on that, I don't think we want to test proximity here.

What we should do here is something like this:

// this might not be easily available right now because we might only get access to Node and not to the NodePath
// Nodes do not hold a reference to their parent in the Babel's AST (in TS they have access to that)
const callExpression = this.machineCallResult.callee.parent;

const callComment = (callExpression.leadingComments || []).findLast((comment) =>
  comment.value.includes("xstate-layout")
);

if (callComment) {
  // construct the return value
  return {};
}

if (t.isVariableDeclarator(callExpression.parent)) {
  const declaratorComment = (
    callExpression.parent.leadingComments || []
  ).findLast((comment) => comment.value.includes("xstate-layout"));

  if (declaratorComment) {
    // construct the return value
    return {};
  }

  // TODO: what should happen if there are multiple matching comments above the declaration?
  const declarationComment = (
    callExpression.parent.parent.leadingComments || []
  ).findLast((comment) => comment.value.includes("xstate-layout"));

  if (declarationComment) {
    // construct the return value
    return {};
  }

  return undefined;
}

if (t.isAssignmentExpression(callExpression.parent)) {
  const assignmentComment = (
    callExpression.parent.leadingComments || []
  ).findLast((comment) => comment.value.includes("xstate-layout"));

  if (assignmentComment) {
    // construct the return value
    return {};
  }

  return undefined;
}

return undefined;

});

if (!layoutComment) return undefined;
Expand Down
39 changes: 39 additions & 0 deletions packages/machine-extractor/src/__tests__/layout-comment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,43 @@ describe('Layout comments', () => {
`layout-string`,
);
});

it('Should parse layout comments that appear after the variable declaration', () => {
const result = extractMachinesFromFile(`
const machine =
/** @xstate-layout layout-string */
createMachine({});
`);

expect(result!.machines[0]!.getLayoutComment()?.value).toEqual(
`layout-string`,
);
});

it('Should parse layout comments that are near variable declaration', () => {
const result = extractMachinesFromFile(`
const machine =
/** @xstate-layout layout-string */

createMachine({});
`);

expect(result!.machines[0]!.getLayoutComment()?.value).toEqual(
`layout-string`,
);
});

it('Should not parse layout comments that are far from machine', () => {
const result = extractMachinesFromFile(`
/** @xstate-layout layout-string */





const machine = createMachine({});
`);

expect(result!.machines[0]!.getLayoutComment()?.value).toBeUndefined();
});
});