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

refactor(maintenance): pkg structure to support subpath exports; TS types and external pkg name #31

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

dreamorosi
Copy link
Collaborator

@dreamorosi dreamorosi commented May 9, 2024

Summary

Changes

Please provide a summary of what's being changed

This PR addresses a series of prerequisites that are needed for the packages to be consumed outside of the workspace (aka after having run npm pack). The PR does not change the logic/implementation of any of the packages.

Prior to this PR all the packages were referencing each others using non-conventional imports which worked while in the same project but would not have worked once the packages are packed and potentially published.

Additionally, the dependencies for each package were not specified at the package level but instead they were listed in the main package.json file. This meant that when installing the packed tarball (i.e. npm i @aws-powertools-actions/github) the dependencies were not being installed since the packed module had no knowledge of

The work for this PR was done in sequential order:

  1. Clear node_modules and package.json

  2. Organize dependencies and devDependencies -> move each runtime dependency from the main package.json to the respective package keeping in mind the dependency tree. For example, since @aws-powertools-actions/reporting depends on @aws-powertools-actions/github, and both use zod, while at the same time @aws-powertools-actions/testing depends on both the other two packages of the workspace as dependency and both of them have the testing package as devDependency

The dependency tree looks like this:

├── @aws-powertools-actions/reporting
│   ├── @aws-powertools-actions/github
│   │   ├── zod
│   │   ├── @octokit/rest
│   │   └── @actions/core
└── @aws-powertools-actions/testing
    ├── @aws-powertools-actions/github
    │   ├── zod
    │   ├── @octokit/rest
    │   └── @actions/core
    └── @anatine/zod-mock

While the devDependency tree looks like this:

├── @aws-powertools-actions/reporting
│   └── @aws-powertools-actions/testing
│       └── @anatine/zod-mock
└── @aws-powertools-actions/github
    └── @aws-powertools-actions/testing
        └── @anatine/zod-mock

As you can see there are cyclical dependencies, but at least for now this is fine since the "bi-directional" dependencies are separate between runtime & test environments.

The only dependencies left in the main package.json are only devDependencies and only those that are shared by 2 or more packages.

  1. Renamed each package in the workspace to a unique name under the @aws-powertools-actions/ namespace (we can change it later up until we publish). This allows us to actually run npm i @aws-powertools-actions/github -w packages/testing to create the dependency tree.

This was necessary because npm under the hood tries to resolve the package from npmjs.com and only if it doesn't find it looks for it locally. With this setup, the packages can depend on each others both while in the workspace and when published.

  1. Reviewed & standardized the structure for each package.json file under packages/*, including all the fields needed to make the package visible to the outside.

  2. Created explicit subpath exports for each module/function that we want to expose.

  3. Reviewed every single import in each file under packages/* to ensure that local packages are referenced with relative paths and correct extension, while external modules (aka cross-package) are referenced using the fully-qualified-name (i.e. @aws-powertools-actions/github) or its corresponding subpath export.

  4. Installed TypeScript and setup centralized tsconfig.json, with local override via extends. At this stage we are not moving to TypeScript and for the short term we might not need to. I have configured TypeScript to still emit type definitions for all the files and made them visible via package.json#exports.types.

The type definitions (aka what's under packages/**/types/*) are not meant to be edited manually but rather generated via npm run build -w packages/<package-name>.

  1. Setup pre-commit hook via husky and together with lint-staged. This way we don't have to remember to build the types before committing changes to the source.

  2. Reviewed Biome config to exclude the autogenerated type definitions and to actually sort imports in the IDE.


As you can see, the diff for this PR is significantly bigger than normal - this is due to: 1/ the types folders which doubled the number of files, and 2/ the fact that I had to touch almost every single file to review the imports.

Regarding the types, strictly speaking we don't have to keep them in version control if we don't want to. Since they're fast & cheap to rebuild we could also keep the source only and setup the project so that they're built on the fly when the repo is cloned/pulled, and the same in CI, as well as when the packages are packed.

Let me know which one you prefer.

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

Issue number: #8


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 May 9, 2024
@dreamorosi dreamorosi requested a review from heitorlessa May 9, 2024 10:38
@heitorlessa
Copy link
Contributor

suuuuuuper! tons for me to learn here; thank you for pulling it together :D

@heitorlessa heitorlessa changed the title chore(maintenance): refactor pkg structure refactor(maintenance): pkg structure to support subpath exports; TS types and external pkg name Jun 10, 2024
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

SHIIIIPPP IT! <3

@dreamorosi dreamorosi merged commit e56b105 into main Jun 10, 2024
5 checks passed
@dreamorosi dreamorosi deleted the chore/refactor_pkg_structure branch June 10, 2024 12:26
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.

3 participants