shelljs / shx

Portable Shell Commands for Node
MIT License
1.72k stars 44 forks source link

Update shelljs to avoid circular dependency warnings on Node 14 #184

Closed Pomax closed 4 years ago

Pomax commented 4 years ago

https://github.com/shelljs/shelljs/pull/973 updated shelljs to prevent Node 14 from flooding the user with hundreds of lines that look like:

(node:117) Warning: Accessing non-existent property 'cd' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'chmod' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'cp' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'dirs' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'pushd' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'popd' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'echo' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'tempdir' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'pwd' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'exec' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'ls' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'find' of module exports inside circular dependency
(node:117) Warning: Accessing non-existent property 'grep' of module exports inside circular dependency

And it would be a good idea to make sure that the minimum version of shelljs matches that updated version.

nfischer commented 4 years ago

This shouldn't be necessary. Semver behavior dictates npm should pick the highest available patch version matching 0.8.*. Try reinstalling shx.

Pomax commented 4 years ago

That's not what you want in this case though: 0.8.4 is effectively a compatibility fix, so you want to peg the minimum version to 0.8.4 and nothing prior to that.

nfischer commented 4 years ago

If anyone downloads shx, they'll already receive shelljs@0.8.4. This is thanks to the ^ operator in semver.

Pomax commented 4 years ago

Not if they rely on cached registries they won't? (e.g. CI/CD systems)

This change makes sure that registry caching sees that it really has to be 0.8.4 or up, and not 0.8.1 or up. Let's keep people's logs readable =(

codecov-commenter commented 4 years ago

Codecov Report

Merging #184 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #184   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          114       114           
=========================================
  Hits           114       114           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b733ec9...55dbc18. Read the comment docs.

nfischer commented 4 years ago

Thank you for explaining your concern for cached registries. My understanding is the cached registry must be updated to choose v0.8.4 as the preferred ShellJS version. I assume cached registries should already have a process for taking patch releases which include bug/compatibility/security fixes.

As a friendly reminder for future contributions, please avoid hyperbolic language to ensure we maintain a respectful discussion. Ex. "spew out 100s of warnings" might have been better phrased as "emit circular dependency warnings." This is maintained in my spare time, so I appreciate respectful language for bug reports and patches.

Pomax commented 4 years ago

Fair enough, although our build log literally had 100s of warnings, that wasn't hyperbole (but pasting 400 line logs is generally not super useful when filing an issue).