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 error due to dependency on @npmcli/arborist #25 #26

Closed
wants to merge 1 commit into from

Conversation

hiro5id
Copy link

@hiro5id hiro5id commented Jun 26, 2021

  • I have remove dependency on @npmcli/arborist.
  • I have extracted the necesary code from arborist that is now no longer available in the latest version of arborist. I could have used @npmcli/package-json which is what latest version of arborist is using now, but that does not behave the same way as the old code, which does not alter the package.json sort order of the properties.

@ultrr
Copy link

ultrr commented Jul 15, 2021

Actually, why is there a need of sorting the package.json? In my opinion, audit command should not change anything except versions of packages, especially when run with '--no-fix' parameter.

@svettwer
Copy link
Collaborator

@ultrr The original problem was that we did not touch the order of the dependencies but it just occurred that we removed new lines at the end of the package.json files because we changed the files by using simply fs. This was changed by #19 due to #15.
I think we introduced more problems that we had before, because now we are talking about reordering dependencies. 😂

To be honest, I don't want to add code that introduces some ordering etc. This would lead to more discussions on the long run for sure. Using dependencies performing certain tasks is one thing but if we add such code to the lib, we would have to maintain it, think about what should be done and what not etc. This would lead to effort which is not in scope of this lib in my opinion.

Nevertheless, we have to get rid of the arborist dependency.

@hiro5id
Copy link
Author

hiro5id commented Aug 20, 2021

Actually, why is there a need of sorting the package.json? In my opinion, audit command should not change anything except versions of packages, especially when run with '--no-fix' parameter.

@ultrr correct me if I'm wrong but I thought that npm install also causes package dependencies to be re-sorted in package.json alphabeticaly if they are not already. I think this project's maintainers thinking to use arborist was to mimic that same behaviour.

@svettwer
Copy link
Collaborator

Hi 👋

I finally found some time to finalize my work on that topic. Long story short: I removed arborist again. That solves the dependency error. In addition, I added some logic to consider basic formatting information of the input files, when writing audited files. That might not be perfect but it does not add so much code to maintain over time.

@svettwer svettwer closed this Sep 13, 2021
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 3, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 3, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 6, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 6, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 7, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 7, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
NSeydoux added a commit to inrupt/solid-client-authn-js that referenced this pull request Dec 7, 2021
tnobody/lerna-audit#26 has been resolved, so lerna-audit does not have a vulnerability anymore.
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.

3 participants