search

thlorenz / browserify-shim

📩 Makes CommonJS incompatible files browserifyable.
MIT License
933 stars 87 forks source link

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

Open ljharb opened 1 year ago

ljharb commented 1 year ago

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 commented 1 year ago

ping @bendrucker :-)

bendrucker commented 1 year ago

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 commented 1 year ago

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

bendrucker commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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