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

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

Closed
1 of 2 tasks
am29d opened this issue Mar 7, 2023 · 7 comments · Fixed by #1738
Closed
1 of 2 tasks

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

am29d opened this issue Mar 7, 2023 · 7 comments · Fixed by #1738
Assignees
Labels
breaking-change Changes that will cause customer impact and need careful triage completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.)
Milestone

Comments

@am29d
Copy link
Contributor

am29d commented Mar 7, 2023

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.

@am29d am29d added triage This item has not been triaged by a maintainer, please wait internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) labels Mar 7, 2023
@dreamorosi
Copy link
Contributor

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.

@dreamorosi dreamorosi added discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Mar 7, 2023
@am29d
Copy link
Contributor Author

am29d commented Mar 8, 2023

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
Copy link
Contributor

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

@am29d am29d added the breaking-change Changes that will cause customer impact and need careful triage label Mar 8, 2023
@dreamorosi
Copy link
Contributor

dreamorosi commented Mar 10, 2023

@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:

  • Add a new package to the workspaces packages/testUtils, this package will not be published to npm for the time being
  • Move all the e2e related utilities, plus the content of the tests (context & events) from commons to testUtils
  • Change all the references in our codebase to point to testUtils
  • Add a @deprecated annotation to the things we'll want to drop informing that they'll be removed in the next major version

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 dreamorosi added this to the Version 2.0 milestone Jun 22, 2023
@dreamorosi dreamorosi added good-first-issue Something that is suitable for those who want to start contributing help-wanted We would really appreciate some support from community for this one confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Sep 26, 2023
@dreamorosi
Copy link
Contributor

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.

@dreamorosi dreamorosi self-assigned this Oct 12, 2023
@dreamorosi dreamorosi removed help-wanted We would really appreciate some support from community for this one hacktoberfest labels Oct 12, 2023
@dreamorosi dreamorosi linked a pull request Oct 12, 2023 that will close this issue
9 tasks
@dreamorosi dreamorosi added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Oct 12, 2023
@github-actions
Copy link
Contributor

⚠️ 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.

@github-actions github-actions bot removed the pending-release This item has been merged and will be released soon label Nov 1, 2023
@github-actions github-actions bot added the completed This item is complete and has been merged/shipped label Nov 1, 2023
@aws-powertools aws-powertools deleted a comment from github-actions bot Nov 2, 2023
@dreamorosi
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes that will cause customer impact and need careful triage completed This item is complete and has been merged/shipped good-first-issue Something that is suitable for those who want to start contributing internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.)
Projects
Development

Successfully merging a pull request may close this issue.

2 participants