-
Notifications
You must be signed in to change notification settings - Fork 10
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: Add @cerbos/react
package
#876
Conversation
e99ce6f
to
f02ff77
Compare
30f85c1
to
c0db125
Compare
@haines where do you think we should test this package? in |
Yeah, that's fine. Although it's maybe less convenient than colocating the tests with the source, the rationale for keeping the tests in a separate package is that it forces us to only test against the publicly-exposed API of the other packages, and it ensures that we don't accidentally ship the tests in the packages we publish to npm. |
3acc41b
to
015577d
Compare
@haines I think docs and the actual code is ready for final review, I'm moving on to testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks great. I've made some minor suggestions for the docs on hasanayan#2. We'll also need to add a changelog but we should do that after #18 lands because I changed the format 🙈
ba27f95
to
c3c7375
Compare
@haines added tests with a placeholder test for testing request cancellation, I'll wait until you merge your PR and I'll add the test. I also added changelog.yml file but I am not sure it's correct. Btw I don't know what should be our initial version, an alpha or beta maybe? For now it's set as 0.0.1 in package.json Also, apparently my brain is still in weekend-low-power mode 😄, I somehow thought your docs PR was actually a direct commit (you don't even have access, I know ^^) and I force pushed and removed it so I manually took those changes in 🤦🏻 But anyway I saved you a rebase 😄 |
be17439
to
de4b51d
Compare
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Andrew Haines <haines@cerbos.dev> Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Andrew Haines <haines@cerbos.dev> Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
@cerbos/react
package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
Signed-off-by: Hasan Ayan <hasanayan@me.com>
Signed-off-by: Hasan Ayan <hasanayan@me.com>
de4b51d
to
59d4860
Compare
@haines all done but; I broke my back trying to get React to complain about setState call after unmounting during tests (without |
Signed-off-by: Hasan Ayan <hasanayan@me.com>
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
expect(abortSignal!.aborted).toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor but here and below you could probably avoid the non-null assertion because the test will still fail if it is undefined:
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |
expect(abortSignal!.aborted).toBe(false); | |
expect(abortSignal?.aborted).toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same but no-unsafe-assignment
rule is not happy about that (no configuration options). I chose this one to suppress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok... weird!
Yeah, sounds painful - I think we have good test coverage of the abort behaviour between the tests I wrote for the client and the one you have added here, so I'm happy enough with that! |
Signed-off-by: Hasan Ayan <hasanayan@me.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
This is an initial/draft PR to agree on some of the concepts. It's lacking documentation and testing and some areas that are subject to change.
I am opening this to be able to discuss the what
@cerbos/react
should offer and some other issues on the environment (linter rules etc)