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

[Deps] switch through to @ljharb/through #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ljharb
Copy link

@ljharb ljharb commented Jul 18, 2023

Closes #249

npm test fails locally because a transitive dev dep, osenv, calls os.tmpDir when the function is now named os.tmpdir, but I'm hoping CI accounts for that.

@ljharb
Copy link
Author

ljharb commented Aug 25, 2023

ping @bendrucker :-)

@bendrucker
Copy link
Collaborator

I'm hoping CI accounts for that

I tried to make that happen in #251. The test suite is badly broken. Having second thoughts on whether this is in a positioned to be maintained versus just frozen/archived.

@ljharb
Copy link
Author

ljharb commented Sep 1, 2023

I'm happy to take a crack at that - I can post a branch link in that PR for you to pull in.

@bendrucker
Copy link
Collaborator

Sure, thank you! The difficulty is that this previously ran on iojs via Travis and wasn't kept up to date. So there's a mountain of dependency upgrades with cascading relationships to handle to get the tests running on the current LTS.

I started going back to try to reproduce the existing test environment including installing iojs. The embedded CA certificates being outdated is understandable but even after disabling TLS verification there are still failing tests.

I just pushed a change fixing the use of t.fail(err) causing massive output on errors with a deeply nested object as a property (stream).

Turns out all that output was causing me to miss this, after the switch to iojs:

2023-09-01T01:40:38.0785630Z [test-peer-range] Passed: 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
2023-09-01T01:40:38.0786305Z [test-peer-range] Failed: 16, 17

So in the short term I'm going to mark the peer dep as <= 15 and then folks can use this at their own risk on later versions. If you or anyone else is up for fixing the issue and re-opening the range, upgrading the tests to modern Node, etc, that's still welcome.

@bendrucker
Copy link
Collaborator

No luck, test-peer-range instead up running npm install browserify which installs latest. That's a bug but this module is so behind that fixing/installing it is a hassle.

I've exhausted the time I can spend on this in the short term. A fix for browserify@16+ would be ideal. Otherwise disabling those tests is potentially the next best option.

@ljharb
Copy link
Author

ljharb commented Sep 1, 2023

Note that changing peer deps is a breaking change.

I'll be happy to post a branch link on your PR in the next few days; hopefully that will get everything up to speed.

@bendrucker
Copy link
Collaborator

Note that changing peer deps is a breaking change.

For these very old versions it's an error if the range doesn't match and so more clearly breaking. I thought that changed to a warning in years since. Part of the module has been broken with browserify@16/17 the whole time so to the extent the peer change is advisory and not mandatory I'd say it's not strictly breaking, at least if engines had required a sufficiently modern Node version where that peer dep handling was the case. Regardless, it's still easier to avoid those semver nuances.

@ljharb
Copy link
Author

ljharb commented Sep 1, 2023

Because it affects npm ls output, it affects every version of npm.

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.

update to a maintained through?
2 participants