-
Notifications
You must be signed in to change notification settings - Fork 48
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
Conversation
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 ```
This shouldn't be necessary. Semver behavior dictates npm should pick the highest available patch version matching |
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. |
If anyone downloads shx, they'll already receive [email protected]. This is thanks to the |
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 Report
@@ 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.
|
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. |
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). |
shelljs/shelljs#973 updated shelljs to prevent Node 14 from flooding the user with hundreds of lines that look like:
And it would be a good idea to make sure that the minimum version of shelljs matches that updated version.