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): fix formatFiles schematic to write changes to correct work… #7944

Merged
merged 1 commit into from Dec 2, 2021

Conversation

philipjfulcher
Copy link
Collaborator

@philipjfulcher philipjfulcher commented Nov 29, 2021

…space config file

Current Behavior

  • If an Angular schematic uses the formatFiles schematic and makes changes to both angular.json and workspace.json, the changes to workspace.json will be written to disk to the correct config file but not the changes to angular.json. For example, if a schematic creates a library using @nrwl/workspace:library and then runs formatFiles, the changes for the library configuration will not be written to angular.json.

@rarmatei has a reproduction repo in #7863

Here is the sequence of events leading to this issue as I understand them:

  1. The the example repo built by @rarmatei , the data-access-lib schematic calls the @nrwl/workspace:lib schematic which correctly updates the angular.json file in the Tree.

  2. The schematic then calls the formatFiles schematic. Part of this schematic includes a function named updateWorkspaceJsonToMatchFormatVersion which takes care of updating the workspace configuration to match the format of the version it's using. This function incorrectly decides that workspace.json is the workspace configuration file and overwrites it in the tree with a formatted version. This incorrect logic is due to both workspace.json and angular.json being present in the schematic's Tree. At this point, both angular.json and workspace.json may have been modified. If the format is already correct, the actual contents of the workspace.json file may not have been modified.

  3. formatFiles then continues to pick up any modified files in the Tree and runs Prettier over them. At this point, both angular.json and workspace.json are flagged for changes and Prettier is run on them. This leaves angular.json with the lib settings and workspace.json with only formatting changes.

  4. The changes are then ready to be committed to disk. The NxScopedHost class takes care of this to map the changes from the schematic's Tree to the actual file system. It correctly determines that the workspace config file is angular.json. Two write commands are submitted to it, one for angular.json and workspace.json. Both of these write commands are mapped to write to the angular.json file. Because workspace.json is written last, it wins, and angular.json is overwritten with just the formatting changes instead of the new library config.

@AgentEnder and I discussed that the root cause may be somewhere in the ngcli-adapter.js file. I'm not sure if that's the case any more. It seems to be correctly determining the right configuration file to write to, but it's being given confusing write commands. By the time the two different write commands reach NxScopedHost, is there enough information to actually tell which update is "right?" Should it merge them together? This seems difficult to do by the time you write to disk, but I'm happy to be wrong about that. It seems like you would want to stop something like this earlier, like in the actual Tree class so that it won't record write actions for both angular.json and workspace.json and instead would pick the right one.

This PR fixes the updateWorkspaceJsonToMatchFormatVersion function that is used by the formatFiles schematic. There's another version of this function in packages/workspace/src/command-line/format.ts used by the command line that correctly picks up the configuration file. Instead of using the Tree, it uses the file system to determine the file name of the configuration file. We can then ensure that changes are only ever requested on the correct workspace config file.

Expected Behavior

It should properly format the file without reverting the changes.

Related Issue(s)

Fixes #7863

@vercel
Copy link

vercel bot commented Nov 29, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/54ufV8DJhLmvy8fC3hWWrHi8jzBb
✅ Preview: Canceled

[Deployment for 5b3790c canceled]

@nx-cloud
Copy link

nx-cloud bot commented Nov 29, 2021

@philipjfulcher philipjfulcher marked this pull request as ready for review December 2, 2021 05:26
@jaysoo jaysoo merged commit f65d6fd into nrwl:master Dec 2, 2021
@philipjfulcher philipjfulcher deleted the fix-format-files branch November 27, 2022 22:41
@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 Mar 16, 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.

formatFiles() reverts changes to angular.json when using the Angular devkit
2 participants