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

fix(core): tree should not be changed after committed to disk in migrations #17071

Conversation

AgentEnder
Copy link
Member

@AgentEnder AgentEnder commented May 17, 2023

Current Behavior

It is hard to debug when a migration writes to the tree after it has been committed. This only occurs when an async statement is not awaited or similar issues with promises, and the migration is assumed to be complete. As such, the warning that you get is in the middle of other migrations and its hard to track down which migration actually has the issue.

This changes how the logs are created to include more context to make the experience nicer.

Expected Behavior

"Changed after committed" warnings include the reason the tree was created.

Related Issue(s)

Fixes #17064

@vercel
Copy link

vercel bot commented May 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 23, 2023 4:34pm

@nx-cloud
Copy link

nx-cloud bot commented May 17, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c0a55f9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from 7627de3 to bcc841f Compare May 17, 2023 20:33
@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from bcc841f to fef4af1 Compare May 17, 2023 20:41
@rcfox
Copy link

rcfox commented May 18, 2023

Maybe it would be worth adding eslint rules to catch these improperly-handled promises as well? :)

    ...
    {
      "files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
      "rules": {
        ...
        "@typescript-eslint/no-floating-promises": "error",
        "@typescript-eslint/no-misused-promises": ["error"],
        ...

@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from fef4af1 to cab2ffa Compare May 18, 2023 21:10
@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from cab2ffa to 54700e4 Compare May 19, 2023 16:43
@@ -360,14 +364,20 @@ export class FsTree implements Tree {
// TODO (v17): Remove condition
if (gt(nxVersion, '17.0.0')) {
throw new Error(
'The tree has already been committed to disk. It can no longer be modified. Do not modify the tree during a GeneratorCallback and ensure that Promises have resolved before the generator returns or resolves.'
`The tree has already been committed to disk ${
Copy link
Member Author

Choose a reason for hiding this comment

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

File changes have already been written to disk...

Go ahead and make this an error.

@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from 54700e4 to f536835 Compare May 19, 2023 19:40
Comment on lines 365 to 367
title: `File changes have already been written to disk. Further changes were attempted ${
this.logOperationId ? `while running ${this.logOperationId}.` : '.'
}`,
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
title: `File changes have already been written to disk. Further changes were attempted ${
this.logOperationId ? `while running ${this.logOperationId}.` : '.'
}`,
title: `File changes have already been written to disk. Further changes were attempted${
this.logOperationId ? ` while running ${this.logOperationId}.` : '.'
}`,

this.logOperationId ? `while running ${this.logOperationId}.` : '.'
}`,
bodyLines: [
'It can no longer be modified. This commonly happens when a generator attempts to make further ',
Copy link
Member

Choose a reason for hiding this comment

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

Now there's no previous mention of the tree. I think the It here lacks context about what it's referring to.

Suggested change
'It can no longer be modified. This commonly happens when a generator attempts to make further ',
'The tree can no longer be modified. This commonly happens when a generator attempts to make further ',

Comment on lines 369 to 370
'It can no longer be modified. This commonly happens when a generator attempts to make further ',
'changes in its callback, or an asynchronous operation is still running after the generator completes.',
Copy link
Member

Choose a reason for hiding this comment

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

Splitting a single sentence into multiple lines can make the sentence split weirdly in the terminal when the terminal is narrower than the parts. I think it's better not to break a sentence into various lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that makes sense.

@AgentEnder AgentEnder force-pushed the fix/add-operation-id-to-changed-after-locked branch from f536835 to c0a55f9 Compare May 23, 2023 16:29
@AgentEnder AgentEnder merged commit f95f8c4 into nrwl:master May 23, 2023
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Tree modified after commit to disk." during migration from v14.8.6 to v16.1.4
3 participants