Skip to content

[arborist] [Fix] build-ideal-tree: ensure indentation is preserved#4218

Merged
ruyadorno merged 3 commits intonpm:release-nextfrom
ljharb:ideal-tree-indent
Jan 18, 2022
Merged

[arborist] [Fix] build-ideal-tree: ensure indentation is preserved#4218
ruyadorno merged 3 commits intonpm:release-nextfrom
ljharb:ideal-tree-indent

Conversation

@ljharb
Copy link
Contributor

@ljharb ljharb commented Jan 7, 2022

It turns out that new Arborist().buildIdealTree().meta.toString() does not take into account the indentation in the package.json (tabs, in my case) the way npm install --package-lock-only does.

This fixes that. I also included a bonus commit that removes redundant Promise stuff inside an async function.

I'm happy to add a test if you suggest where to do so; it seems like it'd require a directory with a package.json that's indented by tabs, and to confirm that the resulting shrinkwrap contents is also indented by tabs.

(related to #4181)

@ljharb ljharb requested a review from a team as a code owner January 7, 2022 05:38
@ljharb ljharb changed the title [arborist] [Fix] build-ideal-tree: ensure indentation is preserved [arborist] [Fix] build-ideal-tree: ensure indentation is preserved Jan 7, 2022
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release ws:arborist Related to the arborist workspace release: next These items should be addressed in the next release labels Jan 11, 2022
ruyadorno
ruyadorno suggested changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

thanks @ljharb! these looks like good catches to me 😊

With regards to the lockfile format, I would prefer to have a more resilient implementation in place while also avoiding some of the unintended repetition. With that in mind I'd favor more refactoring this bit of code from lib/shrinkwrap.js into its own public method e.g: format(source) so that we can make sure we're guarding against undefined values while also making sure we copy over the line break characters:

cli/workspaces/arborist/lib/shrinkwrap.js

Lines 454 to 462 in aa538df

// don't use detect-indent, just pick the first line.
// if the file starts with {" then we have an indent of '', ie, none
// which will default to 2 at save time.
const {
[Symbol.for('indent')]: indent,
[Symbol.for('newline')]: newline,
} = data
this.indent = indent !== undefined ? indent : this.indent
this.newline = newline !== undefined ? newline : this.newline

this way in L337 over here you can just reuse that same root.meta.format(root.package)

This way it's also going to be simpler adding a test to test/shrinkwrap.js that just validates the format() method works as intended. There's already a tab-indented-package-json fixture that can be used in it. I can help with more guidance on how to implement the test if needed so.

Let me know what you think 😄 and thanks again!

@ljharb
Copy link
Contributor Author

ljharb commented Jan 12, 2022

hmm, i'm not sure i understand. The actual formatting isn't being done here - that's just setting the this.indent and this.newline for later usage.

@ljharb ljharb force-pushed the ideal-tree-indent branch from f3fe09f to 19a67d2 Compare January 12, 2022 20:38
@ljharb
Copy link
Contributor Author

ljharb commented Jan 12, 2022 •
edited
Loading

With the recent updates, I can confirm that buildIdealTree is indeed inferring the indentation properly in my npm-lockfile project.

ruyadorno
ruyadorno approved these changes Jan 12, 2022
View reviewed changes
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

awesome 🥳 thank you @ljharb!

@ljharb ljharb force-pushed the ideal-tree-indent branch 2 times, most recently from d2d48ad to 39d0aff Compare January 16, 2022 02:57
@ruyadorno ruyadorno merged commit 510f0ec into npm:release-next Jan 18, 2022
@ljharb ljharb deleted the ideal-tree-indent branch January 18, 2022 19:26
This was referenced Jan 19, 2022
Closed
Closed
@lukekarrys lukekarrys mentioned this pull request Jan 20, 2022
Merged
@npm-robot npm-robot mentioned this pull request Jan 20, 2022
Closed
ljharb added a commit to ljharb/npm-lockfile that referenced this pull request Jan 20, 2022
@ljharb
[Refactor] update @npmcli/arborist; remove indentation workaround
e9dc524 
See npm/cli#4218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ruyadorno ruyadorno ruyadorno approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes ws:arborist Related to the arborist workspace

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ljharb @ruyadorno @darcyclarke