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

Release 4.18.3 #5505

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Release 4.18.3 #5505

merged 1 commit into from
Feb 29, 2024

Conversation

UlisesGascon
Copy link
Member

@UlisesGascon UlisesGascon commented Feb 26, 2024

This is a tracking issue for release 4.18.3

Please keep feature requests in their own issues

If you want to make a comment on a particular change, please make the comment in the "Files changed" tab so comments are not lost during a rebase.

List of changes for release:

  • Fix routing requests without method
  • deps: body-parser@1.20.2
    • Fix strict json error message on Node.js 19+
    • deps: content-type@~1.0.5
    • deps: raw-body@2.5.2

Testing this release

If you want to try out this release, you can install it with the following commands:

$ npm cache clean express
$ npm install expressjs/express#4.18.3-staging

Owners/collaborators: please do not merge this PR :)

@UlisesGascon UlisesGascon requested a review from a team February 26, 2024 19:28
@UlisesGascon UlisesGascon marked this pull request as ready for review February 26, 2024 19:28
@UlisesGascon
Copy link
Member Author

UlisesGascon commented Feb 26, 2024

As most of the changes are already in master it is quite hard to see the changes in this PR, but here is a list compared to 4.18.2:

@rluvaton
Copy link
Contributor

Hey @UlisesGascon I didn't realize you are contributing to express as well, I thought I got node release notification 😆

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is the first release prep we are doing with the new TC members, I just wanted to comment and thank @UlisesGascon for sorting this all out!

In the future we will update the documentation for this process to make sure it is approachable, but for now we were working on ensuring it followed the process as best we could. If anyone sees something we missed (cc @dougwilson) please let us know here.

@jonchurch
Copy link
Member

Here's the Compare view link if you want an autoupdating and clean diff/commit list: 4.18.2...4.18.3-staging

@jonchurch
Copy link
Member

jonchurch commented Feb 27, 2024

@UlisesGascon if you still want to block merging on this until it is ready, can I suggest converting it to a draft or leaving a blocking comment as a review yourself with what is still to be done? I missed your comment of

Owners/collaborators: please do not merge this PR :)

so was wondering why it wasn't merged if it's approved. Im not able to merge, so not risk there to me specifically. This is a process nit, but I suggest using the tools we have natively in github to communicate readiness of PRs.

This is for bus factor, async collaboration reasons, and legibility. Someone can always log on at 3am and click merge bc everything is green. No harm here if that's done, but let's not make it that easy for folks to do the opposite of what you've asked! An owner can always switch it from draft to published, and merge anyways, but that's a lot harder to do by accident.

@wesleytodd
Copy link
Member

wesleytodd commented Feb 27, 2024

Yeah I agree with @jonchurch long term. What @UlisesGascon and I chatted about in slack was more about "lets try and replicate exactly what Doug would do in the past", hence attempting to follow the full form. The previous example is here:

#4287

@jonchurch
Copy link
Member

Ah! I missed that context, understood ❤️❤️

@sheplu
Copy link
Member

sheplu commented Feb 27, 2024

Amazing work!
I may be missing something but where is it published ? I am not seeing in on the npm express page

It could be interesting to have a quick chat with the team behind sails / nestjs / nextjs (?) and other big framework that rely on express, just to check with them if they can run their test against this version

@UlisesGascon
Copy link
Member Author

I may be missing something but where is it published ?

If we are happy with the changes in this PRs (2-3 active TC members approval?), I will merge the PR and then generate the tag, release and finally publish it on NPM 👍.

It could be interesting to have a quick chat with the team behind sails / nestjs / nextjs (?) and other big framework that rely on express, just to check with them if they can run their test against this version

Agree, but not sure if we have already all the comms in place with them. Also this won't be a breaking change, so I assume that we can release it without waiting. But definitively we will need to have it in the future.

@UlisesGascon UlisesGascon requested a review from a team February 27, 2024 06:59
Copy link
Member

@sheplu sheplu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wesleytodd
Copy link
Member

I think ideally we smooth these things out as we move forward and this lands pretty much as is. So I think my proposal is that we take these conversations into the discussions repo so we can improve the process in the future and we just land this. I was going to try and test it in a project or two today, but this is low risk and so I wouldn't want little things to block it.

@UlisesGascon
Copy link
Member Author

UlisesGascon commented Feb 27, 2024

I will do the release tomorrow on Feb 29th unless there is any blocker or change request before that 🙂

@UlisesGascon UlisesGascon self-assigned this Feb 27, 2024
@UlisesGascon UlisesGascon merged commit 1b51eda into master Feb 29, 2024
52 checks passed
@UlisesGascon
Copy link
Member Author

Release has been completed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants