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

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

Merged
merged 1 commit into from Aug 4, 2020
Merged

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

merged 1 commit into from Aug 4, 2020

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Jul 28, 2020

shelljs/shelljs#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.

shelljs/shelljs#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
```
@nfischer
Copy link
Member

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

@Pomax
Copy link
Contributor Author

Pomax commented Jul 29, 2020

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
Copy link
Member

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

@Pomax
Copy link
Contributor Author

Pomax commented Aug 1, 2020

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
Copy link

codecov-commenter commented Aug 4, 2020

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 nfischer changed the title Update shelljs to note cause Node 14 to spew out 100s of warnings Update shelljs to avoid circular dependency warnings on Node 14 Aug 4, 2020
@nfischer nfischer merged commit e9e2ba1 into shelljs:master Aug 4, 2020
@nfischer
Copy link
Member

nfischer commented Aug 4, 2020

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
Copy link
Contributor Author

Pomax commented Aug 6, 2020

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).

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.

None yet

3 participants