feat(pkg-manifest): preserve comments in json5 manifests #5677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zkochan
requested changes
Nov 25, 2022
| @@ -118,9 +119,72 @@ export async function tryReadProjectManifest (projectDir: string): Promise<{ | |||
| } | |||
|
|
|||
| function detectFileFormatting (text: string) { | |||
| const finalNewline = text.endsWith('\n') | |||
| if (!finalNewline) { | |||
There was a problem hiding this comment.
move this new logic to a new function and don't run it for JSON files.
| .stringify(manifest, undefined, opts?.indent ?? '\t') | ||
|
|
||
| if (opts?.comments) { |
There was a problem hiding this comment.
Move this new logic to a function and don't run it for JSON files.
Use npm package `strip-comments-strings` to find all comments in any manifest file as it is read. Save them as part of the "file formatting" detected by the manifest reader, noting for each the text of the lines they are on, before, and after, and the line number as a fallback. When the manifest is written, attempt to place each comment back in the resulting JSON5 text, so that the text of the line it is on, before, or after (in that priority order) matches the text at time of reading. Otherwise, so that no comments are lost, replace the comment on the same line number, adding a notation that it may have been relocated (due to sorting dependencies, for example, it may no longer be in the same "logical position" in the file, even though it is on the same physical line number). When comments are in fairly ordinary positions and the manifest does not change too drastically (i.e. once the dependencies are sorted as pnpm prefers, and there are not many parameters added all at once), this strategy results in exact preservation of the comments, as a new test shows. The motivation for this commit is to take advantage of the feature of JSON5 that it allows human-readable comments. For this feature to be useful in the case of package.json5 manifests, those comments must be preserved across manifest changes. Partially resolves pnpm#2008. [That issue requests comment preservation also for YAML manifests, but I have no experience working with YAML, and it also requests that key order be preserved, but I did not address key order because current code in the the pnpm manifest reader/writer _explicitly_ reorders keys -- clearly deliberately -- so I did not want to simply remove code that appeared to have been purposefully written and included.]
(Including one unused variable that was inadvertently left in.)
Move comment extraction and reinsertion into their own functions, called only when the manifest is Json5.
gwhitney
force-pushed
the
preserve_json5_comments
branch
from
c785009
to
7fba4af
Compare
|
I think that should take care of your current requests. Thanks for reviewing. |
zkochan
approved these changes
Nov 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use npm package
strip-comments-stringsto find all comments in anymanifest file as it is read. Save them as part of the "file formatting"
detected by the manifest reader, noting for each the text of the lines
they are on, before, and after, and the line number as a fallback.
When the manifest is written, attempt to place each comment back in
the resulting JSON5 text, so that the text of the line it is on,
before, or after (in that priority order) matches the text at time of
reading. Otherwise, so that no comments are lost, replace the comment
on the same line number, adding a notation that it may have been
relocated (due to sorting dependencies, for example, it may no longer
be in the same "logical position" in the file, even though it is on
the same physical line number).
When comments are in fairly ordinary positions and the manifest does
not change too drastically (i.e. once the dependencies are sorted as
pnpm prefers, and there are not many parameters added all at once),
this strategy results in exact preservation of the comments, as a new
test shows.
The motivation for this commit is to take advantage of the feature of
JSON5 that it allows human-readable comments. For this feature to be
useful in the case of package.json5 manifests, those comments must be
preserved across manifest changes.
A cleaner implementation of this facility would parse the json5 manifest,
attaching comments as metadata to the proper nodes of the syntax tree.
However, I am unaware of a json5 parser that does that, and certainly the
parser currently being used does not. So hopefully this pragmatic
approach will be acceptable for production use.
Partially resolves #2008. [That issue requests comment preservation
also for YAML manifests, but I have no experience working with YAML,
and it also requests that key order be preserved, but I did not
address key order because current code in the the pnpm manifest
reader/writer explicitly reorders keys -- clearly deliberately --
so I did not want to simply remove code that appeared to have been
purposefully written and included.]