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

chore: split packages/access-client/src/capabilities to new package @web3-storage/capabilities #218

Merged
merged 20 commits into from
Nov 30, 2022

Conversation

gobengo
Copy link
Contributor

@gobengo gobengo commented Nov 28, 2022

Motivation:

To do:

  • initial package split
  • remove capabilities dir from packages/access-client and fix imports in access-client
  • fix imports from other packages that were importing from access-client/capabilities
  • consider splitting to capabilities/{service}/types not capabilities/types
    • decided not to unless requested (makes exports map even more complicated, can do later)
  • GH actions should run tests on packages/capabilities
  • .github/workflows/release.yml does npm/docs jobs if paths_released contains packages/capabilities

@gobengo gobengo temporarily deployed to dev November 28, 2022 21:48 Inactive
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 29, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5f97d52
Status: ✅  Deploy successful!
Preview URL: https://6b9d7eb3.ucan-protocol.pages.dev
Branch Preview URL: https://1669667995-caps-package.ucan-protocol.pages.dev

View logs

@gobengo gobengo self-assigned this Nov 29, 2022
@gobengo gobengo temporarily deployed to dev November 29, 2022 22:57 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:02 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:04 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:13 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:23 Inactive
@gobengo gobengo marked this pull request as ready for review November 29, 2022 23:25
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:30 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:31 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:33 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:35 Inactive
@gobengo gobengo temporarily deployed to dev November 29, 2022 23:43 Inactive
@gobengo gobengo temporarily deployed to dev November 30, 2022 01:19 Inactive
@gobengo gobengo temporarily deployed to dev November 30, 2022 19:41 Inactive
@gobengo gobengo temporarily deployed to dev November 30, 2022 19:43 Inactive
@gobengo gobengo temporarily deployed to dev November 30, 2022 19:47 Inactive
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Most comments are suggestions, feel free to address or disregard. I don't think we need another review cycle even if you choose to address them.

.github/workflows/capabilities.yml Show resolved Hide resolved
.github/workflows/capabilities.yml Outdated Show resolved Hide resolved
packages/access-api/src/kvs/spaces.js Show resolved Hide resolved
packages/access-api/src/service/index.js Outdated Show resolved Hide resolved
packages/access-client/src/agent.js Outdated Show resolved Hide resolved
packages/capabilities/package.json Outdated Show resolved Hide resolved
packages/capabilities/src/index.js Show resolved Hide resolved
@gobengo gobengo temporarily deployed to dev November 30, 2022 21:09 Inactive
Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
@gobengo gobengo temporarily deployed to dev November 30, 2022 21:35 Inactive
@gobengo gobengo temporarily deployed to dev November 30, 2022 21:42 Inactive
@gobengo gobengo merged commit 50a50cf into main Nov 30, 2022
@gobengo gobengo deleted the 1669667995-caps-package branch November 30, 2022 21:48
Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

made a PR with fixes for my inline comments #239

@@ -28,7 +28,7 @@ jobs:
node-version: 18
cache: 'pnpm'
- run: pnpm install
- run: pnpm run build
- run: pnpm -r --filter @web3-storage/access run build
Copy link
Contributor

Choose a reason for hiding this comment

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

actually now it need to be pnpm run build because access-client depends on capabilities

Comment on lines +21 to +28
strategy:
matrix:
pnpm-version:
- 7
node-version:
- 18
package:
- capabilities
Copy link
Contributor

Choose a reason for hiding this comment

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

why matrix ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's what @Gozala had done in the code snippet he linked me to in his review :)

Comment on lines +39 to +42
- name: Typecheck
uses: gozala/typescript-error-reporter-action@v1.0.8
with:
project: packages/${{matrix.package}}/tsconfig.json
Copy link
Contributor

Choose a reason for hiding this comment

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

i really do not like this action, it makes reviews harder if there are any ts errors, like reviewing a draft PR and pnpm run lint already does typechecking

uses: gozala/typescript-error-reporter-action@v1.0.8
with:
project: packages/${{matrix.package}}/tsconfig.json
- run: pnpm -r --filter @web3-storage/${{matrix.package}} run lint
Copy link
Contributor

Choose a reason for hiding this comment

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

needs - run: pnpm -r --filter @web3-storage/${{matrix.package}} run build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I removed it based on this

@@ -28,7 +28,7 @@ jobs:
node-version: 18
cache: 'pnpm'
- run: pnpm install
- run: pnpm run build
- run: pnpm -r --filter @web3-storage/upload-client run build
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this please, running build for all packages catches errors that local build doesnt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like you reverted it. lmk if there's something more I should do

Copy link
Contributor

Choose a reason for hiding this comment

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

no all of it is in #239

Comment on lines +19 to +20
"test:node": "mocha 'test/**/!(*.browser).test.js' -n experimental-vm-modules -n no-warnings",
"test:browser": "playwright-test 'test/**/!(*.node).test.js'",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have browser or node specific tests in this package ?

},
"eslintConfig": {
"extends": [
"./node_modules/hd-scripts/eslint/preact.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be "./node_modules/hd-scripts/eslint/index.js"

gobengo added a commit that referenced this pull request Apr 11, 2023
…web3-storage/capabilities (#218)

Motivation:
* #80

To do:
* [x] initial package split
* [x] remove capabilities dir from packages/access-client and fix
imports in access-client
* [x] fix imports from other packages that were importing from
access-client/capabilities
* [x] consider splitting to `capabilities/{service}/types` not
`capabilities/types`
* decided not to unless requested (makes exports map even more
complicated, can do later)
* [x] GH actions should run tests on packages/capabilities
* [x] .github/workflows/release.yml does npm/docs jobs if paths_released
contains packages/capabilities

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants