express icon indicating copy to clipboard operation
express copied to clipboard

deps: encodeurl@~2.0.0

Open blakeembrey opened this issue 1 month ago • 6 comments

This should be an improved and more flexible fix over https://github.com/expressjs/express/commit/0b746953c4bd8e377123527db11f9cd866e39f94. No more parsing required, but it does change the encoding behavior of 3 characters (\, |, ^). They will no longer be encoded. I'm considering this a continuation of the bug fix, although it introduces a potential breaking change for any one that might have been relying on \ to be encoded in the location header without having encoding it themselves. I believe the impact should be negligible to non-existent, but it's not possible to confirm before landing this fix.

This fix is preferred over the existing patch in master because it'll better handle all alternative formats of URLs and encode them consistently.

blakeembrey avatar Mar 29 '24 00:03 blakeembrey

Im gonna hack on a specific test to see if I can break it, but Im for this.

Edit: wasnt able to fail the one I was interested in after hacking around

jonchurch avatar Mar 29 '24 23:03 jonchurch

We didn't talk about this at the @expressjs/express-tc meeting, but I am still down with this change and cutting a new release for it.

jonchurch avatar Apr 11 '24 19:04 jonchurch

@jonchurch would you want to run this release? I was in london last week and so didn't want to do it without the right attention given and haven't had a chance yet to follow up on it. Also, I was hoping we would rotate the job of running releases so if you wanted to run this I am happy to help.

wesleytodd avatar Apr 11 '24 20:04 wesleytodd

Im down for it, had a packed week so opted outside instead of releasing! I went to go do it over the weekend but noticed appveyor is sleeping, so I stopped and tried to rewrite our windows testing. Hence #5599, but that needs more work. AKA didn't want to merge without seeing green even though we should be G2G on this change w/r/t windows.

Don't let me block though, happy to see this get out ASAP.

jonchurch avatar Apr 15 '24 15:04 jonchurch

This will also resolve https://github.com/expressjs/express/issues/5602.

blakeembrey avatar Apr 16 '24 21:04 blakeembrey

We consider this as a minor change or a patch?

Preferably minor, it can be bundled with https://github.com/expressjs/express/pull/5603, which is also a minor version bump.

blakeembrey avatar Apr 24 '24 22:04 blakeembrey

this has consensus and patina, merging

jonchurch avatar May 04 '24 20:05 jonchurch