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

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

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

Conversation

davidkpiano
Copy link
Member

This PR ensures that the machine extractor will detect the layout string even if it's not directly above the const declaration. This potentially solves the multiple layout string issues found here:

@davidkpiano davidkpiano requested a review from Andarist January 8, 2023 13:48
@changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2023

⚠️ No Changeset found

Latest commit: 052e14d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines +341 to +344
const proximity =
this.machineCallResult.callee.loc!.start.line - comment.loc!.end.line;

return Math.abs(proximity) <= 2;
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;

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.

2 participants