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

feat(internal): support Middy.js 5.x #2748

Merged
merged 4 commits into from
Jul 10, 2024
Merged

feat(internal): support Middy.js 5.x #2748

merged 4 commits into from
Jul 10, 2024

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

This PR adds support for Middy.js 5.x by including it in some of the integration test cases that are run on the utilities and widening the version range in the peer dependencies from >= 4.x to 4.x || 5.x.

Because we want to continue supporting both versions, I decided to use an npm feature that allows you to install a package under a custom alias. This mechanism allows multiple versions of the same-name package side-by-side, which is what we need in this case.

To modify the devDependencies of the project I run these commands:

npm un @middy/core # remove existing version
npm i -D middy4@npm:@middy/[email protected]
npm i -D middy5@npm:@middy/[email protected]

This created two entries in the main package.json file, and allows us to import either of the two versions using the name of the alias like so:

import middy from 'middy5'; // this will resolve to the node_moduels/middy5/* contents

With this, I switched some of the test cases that were already running using ESM output to import v5, and left the others untouched.

I also updated the docs to reflect that we support both versions.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: #2049


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Jul 9, 2024
@dreamorosi dreamorosi requested review from a team as code owners July 9, 2024 10:58
@boring-cyborg boring-cyborg bot added dependencies Changes that touch dependencies, e.g. Dependabot, etc. documentation Improvements or additions to documentation tests PRs that add or change tests labels Jul 9, 2024
@pull-request-size pull-request-size bot added the size/S PR between 10-29 LOC label Jul 9, 2024
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jul 9, 2024
@dreamorosi dreamorosi linked an issue Jul 9, 2024 that may be closed by this pull request
2 tasks
Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Great addition of middy5! Few questions:

With this, I switched some of the test cases that were already running using ESM output to import v5, and left the others untouched.

Does this imply that not all e2e tests that can run with middy5 are covered and we need to add them overtime? Will this result in duplication?

I just realised that we need to fix our peerDeps for parser as well :| will do in a separate PR.

@dreamorosi
Copy link
Contributor Author

Does this imply that not all e2e tests that can run with middy5 are covered and we need to add them overtime?

Currently we have a number of e2e test cases (i.e. 10 <- fictional number for the sake of the argument). Of these 10, today we are running some of them (i.e. 5) using ESM and some other (i.e. the other 5) with CJS.

Generally speaking, the distribution between the two has been done in a way that each utility has at least 1 or more tests running in each mode.

Because of Middy.js 5.x only supporting ESM, the pool of eligible test cases where to import middy5 is reduced to these cases only. On the other hand, generally speaking each utility only has one test group (i.e. this is one) that runs tests on functions using Middy.

With all this in mind, the changes in the PR ensure that at least one group of tests for each utility is testing Middy.js 5. I didn't add any new test, and I don't think we should do so, because we are already testing different combinations when looking at the tests all together.

@pull-request-size pull-request-size bot added size/M PR between 30-99 LOC and removed size/S PR between 10-29 LOC labels Jul 10, 2024
@dreamorosi
Copy link
Contributor Author

I just realised that we need to fix our peerDeps for parser as well :| will do in a separate PR.

Added in the last commit - good catch!

@dreamorosi dreamorosi requested a review from am29d July 10, 2024 09:08
Copy link

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Great work as always, I am sure the middy5 support release will be a surprise for many 😎

@am29d am29d merged commit 1d7ad61 into main Jul 10, 2024
13 checks passed
@am29d am29d deleted the feat/support_middy5 branch July 10, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes that touch dependencies, e.g. Dependabot, etc. documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes size/M PR between 30-99 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Support Middy 5.x
2 participants