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

Cannot set property reason of #<AbortSignal> which has only a getter #28

Closed
Koka opened this issue Feb 17, 2022 · 6 comments
Closed

Cannot set property reason of #<AbortSignal> which has only a getter #28

Koka opened this issue Feb 17, 2022 · 6 comments
Labels
bug Something isn't working

Comments

@Koka
Copy link

Koka commented Feb 17, 2022

First of all, thanks for your amazing framework!

Unfortunately recently I started to get error "Cannot set property reason of AbortSignal which has only a getter" from this code line while trying to discard a cancellation token. This was working in previous Chrome versions, but stopped at some point.

My browser is Google Chrome 98.0.4758.80 (arm on M1) and apparently AbortSignal.reason is a readonly field and can't be assigned.

Any help would be appreciated as now I have to use some ugly code workarounds to propagate cancellation reason through my entire flow.

@getify
Copy link
Owner

getify commented Feb 17, 2022

When AbortSignal first came out, they had no reason or ability to set it with the abort(..) call. I considered that a huge failure of design so I added it with CAF. It appears now that they've added it, so now CAF is in conflict. :(

I'll figure out a way to re-wire CAF so it doesn't conflict.

@getify getify added the bug Something isn't working label Feb 17, 2022
getify added a commit that referenced this issue Feb 17, 2022
…tive AbortSignal (in browsers) has recently added 'reason', which CAF was previously patching/extending, per #28
@getify
Copy link
Owner

getify commented Feb 17, 2022

I've hopefully patched this bug with a successful fix (much more involved than I expected!!!).

I did a pre-release on npm of CAF, at version 15.0.0-preA. Can you install that and see if it indeed works and fixes the issue you were seeing?

@Koka
Copy link
Author

Koka commented Feb 18, 2022

Thanks! I've tested 15.0.0-preA and it apparently works, there's no error anymore and cancellation reason seems to be propagated properly.

Is it safe to use this "preA" version or should I wait for a real "15.0.0" one?

@getify
Copy link
Owner

getify commented Feb 18, 2022

I will release a full 15.0.0 hopefully today. I wanted to validate that it fixed your concerns, but also to make sure the docs explain the necessary bits of this change. I think I have a few more tweaks to do on that.


Note that there is still risk in this update, because I can't as fully test this as I would prefer. The test suite runs and passes in both Node and in my Chrome... but since it's very browser dependent, it might not work fully correctly in Firefox or some other browser.

In particular, my test coverage reporting (using NYC/Istanbul) only works in Node, so I only know what lines are actually being tested there. I have visually reviewed all the uncovered lines/branches, and they all seem correct for what the browser will expect. But... shrugs?!

It may be prudent to wait a few days and make sure we're testing CAF in more places than just one version of Chrome.

getify added a commit that referenced this issue Feb 25, 2022
…ecent versions of Node (16.11.1 vs 16.14.0 vs 17.5.0), per #28
@getify
Copy link
Owner

getify commented Feb 25, 2022

So, I discovered there were still some issues with the reason behavior in various recent versions of Node (tested: 16.4.0, 16.11.1, 16.14.0, and 17.5.0). Annoyingly, AbortSignal landed the reason but did not land the default DOMException behavior when reason is missing, and then later they added that extra behavior. That makes proper feature testing of this a lot more complicated. :(

I think this set of changes fixes that.

@Koka I've just released 15.0.0-preB (not preA) with the changes. Could I ask you to re-verify if this new pre-release version still works properly? I don't wanna assume it still works just because the test suite passes, and I don't wanna miss any more of these variations.

@Koka
Copy link
Author

Koka commented Feb 25, 2022

@getify 15.0.0-preB seems to be working properly too in my environment, just as preA one was

@Koka Koka closed this as completed Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants