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

feat(storage-browser): redefine createStorageBrowser interface #5907

Open
wants to merge 3 commits into
base: feat-storage-browser/main
Choose a base branch
from

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Oct 15, 2024

Description of changes

  • created a new set of interfaces to support the createStorageBrowser API
  • now generating view interfaces based on custom actions
  • now generating view sub-component interfaces based on action types
  • necessary change made to actions/configs/types.ts to ensure contextual typing works
  • Introduced jest-tsd to unit testing the type generation

Issue #, if available

Description of how you validated changes

  1. manual testing
  2. unit tests

Checklist

  • Have read the Pull Request Guidelines
  • PR description included
  • yarn test passes and tests are updated/added
  • PR title and commit messages follow conventional commit syntax
  • If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, e2e, examples, or other private packages.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@HuiSF HuiSF requested a review from a team as a code owner October 15, 2024 21:56
Copy link

changeset-bot bot commented Oct 15, 2024

⚠️ No Changeset found

Latest commit: 566972b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Comment on lines +62 to +63
"jest-tsd": "^0.2.2",
"@tsd/typescript": "^5.1.6",
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to run jest-tsd.

"devDependencies": {
"jest-tsd": "^0.2.2",
"@tsd/typescript": "^5.1.6",
"@types/node": "^18.19.50"
Copy link
Member Author

Choose a reason for hiding this comment

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

jest-tsd relying on node types, the workspace has had lower version of @types/node transitively, but versions lower. than 18.19.5 causes a typescript error while running the test. So using "@types/node": "^18.19.50" here.

@HuiSF HuiSF changed the title feat(storage-browser): redefine createStorageBrowser ineterface feat(storage-browser): redefine createStorageBrowser interface Oct 15, 2024
- Generate interfaces based on custom actions
@HuiSF HuiSF force-pushed the feat-storage-browser/custom-action-types branch from 3a43aff to 67cf7b4 Compare October 15, 2024 21:59
cshfang
cshfang previously approved these changes Oct 15, 2024
Copy link
Member

@calebpollman calebpollman left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @HuiSF! Just did a quick review until i pull it down locally, mostly naming feedback and a couple questions

Comment on lines -83 to +87
isCancelable: IsCancelableTaskHandler<T>;
isCancelable: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

This change breaks restricting isCancelable based on the allowed result of the provided handler.

For example, if i provide a handler with a CancelableTaskHandlerOutput, the value of result is :

"CANCELED" | "COMPLETE" | "FAILED"

IsCancelableTaskHandler will restrict isCancelable to true. For context, isCancelable is ultimately a runtime flag used to determine whether or not to display the "Canceled" count in the StatusDisplay

Copy link
Member Author

Choose a reason for hiding this comment

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

This restriction makes isCancelableTaskHandler field un-configurable while defining a custom action, where the result is not a part of the custom action interface to infer the correct value from. (with the restriction, isCancelableTaskHandler value will always be false within custom actions) Do we need an interface change for this?

Copy link
Member

Choose a reason for hiding this comment

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

In my local testing on feat-storage-browser/main that is not the case, although i did need to make the change you applied here

Copy link
Member

Choose a reason for hiding this comment

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

But also open to interface changes if need be 😄

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