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

fix(middleware): add @middy/core as dependency #1218

Conversation

obiwabrakenobi
Copy link

@obiwabrakenobi obiwabrakenobi commented Jan 10, 2023

Description of your changes

As middy is only added as dev dependency any projects using this library with typescript tsc fails to compile as is requires middy as dependency. So you need to manually add it to your project. Adding it as dependency to each of the packages installs the required middy package when running npm install.

How to verify this change

The package.json of the respective workspaces have added middy (updated) as dependency and its being removed from the dev dependency.

Related issues, RFCs

Issue number: #1080

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

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

@pull-request-size pull-request-size bot added the size/XS PR between 0-9 LOC label Jan 10, 2023
@dreamorosi
Copy link
Contributor

Hi @obiwabrakenobi, thank you for taking the time to open the PR and propose a solution, however I don't think this is the fix we are looking for.

Making @middy/core a dependency means that it will be included and bundled for all users of Powertools, even those who are not actually using Middy. This used to be the case in the past, but Middy was moved to devDependency in #511 to avoid this.

A solution for #1080 should account for @middy/core being a devDependency and allow for tsc to correctly bundle when it's not installed.

@dreamorosi dreamorosi added the do-not-merge This item should not be merged label Jan 10, 2023
@obiwabrakenobi
Copy link
Author

Yes I understand your argument. I found it reasonable to optimize the DX by installing the dependency right away compromising the costs with bundling. As far as I saw middy/core is pretty small. Adding it as dev dependency is further a confusing fact because after installing the build fails.

The only other option I see so far is to create additional packages for the respective middleware’s. If you want I can refactor the packages towards isolated packages for the middy middleware.

@dreamorosi
Copy link
Contributor

Yes, I see your point, but as per our tenets (keep it lean) we want to minimize the amount of dependencies no matter the size.

We are having a discussion under #1068 about what would be the best way forward, see my latest comment.

Please feel free to follow & contribute to the discussion under that thread. Once we all come to an agreement I'd be more than happy to accept & review a PR from you with the agreed fix.

What do you think?

@dreamorosi
Copy link
Contributor

I will close the PR for now, we can reopen it or create a new one once we have a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge This item should not be merged size/XS PR between 0-9 LOC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants