aws-powertools / powertools-lambda-typescript

Powertools is a developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/typescript/latest/
MIT No Attribution
1.54k stars 134 forks source link

Maintenance: remove tests folder from the commons dist package #1354

Closed am29d closed 11 months ago

am29d commented 1 year ago

Summary

[!important] Please refer to this comment down below for the latest updates on the scope & tasks for this issue.

In our commons package we export sample resources, which are bundled in our distribution package. We should remove these resources from the distribution package.

Why is this needed?

These sample resources are unnecessary for the production build.

Which area does this relate to?

No response

Solution

No response

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

dreamorosi commented 1 year ago

Do we treat this as a breaking change?

I agree that they could/should be removed, but we don't know if anyone is depending on them already.

am29d commented 1 year ago

Yes, since the code was available to customers. We use the code only in our tests across most packages. Moving the code to commons/tests would require import additional exports. Alternatively we can create a dedicated testing package to move these kind of utilities and e2e related code.

dreamorosi commented 1 year ago

Alternatively we can create a dedicated testing package to move these kind of utilities and e2e related code.

I think that's the way to go, we should create a new package in the workspace and extract those things not needed at runtime there. Also I don't think we should publish this pkg to npm for now.


About the breaking change part, this means we'll have to wait till 2.0

dreamorosi commented 1 year ago

@am29d, do you think we should create a separate issue to create, in due time, a the new unpublished package & move over all the internal references to it while also marking the published one as deprecated?

In this new issue we could:

This way we can avoid adding more things inside commons that don't belong there, ease the migration for customers, and also make our life easier when v2 approaches.

Obviously this wouldn't be a priority issue, but I think we could approach it before v2 discussions.

At the same time, this issue here (#1354) would stay open as-is and be closed once we actually remove the files.

dreamorosi commented 11 months ago

As it stands, the only actions left for this issue are:

  1. Take helloworldContext (here) and rename it to dummyContext. Then move it under packages/testing and export it. This way it can be continued to be used in the unit tests of other utilities.

  2. Remove the folder in packages/commons/src/samples (here) and all its contents, as well as all its mentions (i.e. in tests) & exports.


If you are interested in working on this issue, please leave a comment below so we can assign it to you. If you have any additional question before claiming the issue, feel free to ask either here on in our Discord server under the #typescript channel.

github-actions[bot] commented 11 months ago

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

dreamorosi commented 10 months ago

This is available in preview starting from the 2.0.0-alpha.0 release. You can install this version using the next tag, i.e. npm i @aws-lambda-powertools/logger@next.