Replace abort-controller that has no update 7 years with React Native fork#57230
Replace abort-controller that has no update 7 years with React Native fork#57230retyui wants to merge 2 commits into
abort-controller that has no update 7 years with React Native fork#57230Conversation
c73abf7 to
bc9cea8
Compare
bc9cea8 to
c64a914
Compare
|
@rubennorte could you please review this PR ? |
1 similar comment
|
@rubennorte could you please review this PR ? |
| @@ -0,0 +1,318 @@ | |||
| /** | |||
There was a problem hiding this comment.
We've soft-deprecated normal Jest tests for React Native runtime APIs. Please migrate to Fantom (using the -itest suffix).
There was a problem hiding this comment.
Also, can we just call this AbortController-itest?
|
@rubennorte has imported this pull request. If you are a Meta employee, you can view this in D110042076. |
rubennorte
left a comment
There was a problem hiding this comment.
Thanks for working on this. Please address the feedback so we can merge it.
| * Docs: https:developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static | ||
| * Spec: https://dom.spec.whatwg.org/#dom-abortsignal-timeout | ||
| */ | ||
| static timeout(timeInMs: number): AbortSignal { |
There was a problem hiding this comment.
This should also have a static abort method on AbortSignal
There was a problem hiding this comment.
you are right, I missed it
| static timeout(timeInMs: number): AbortSignal { | ||
| if (!(timeInMs >= 0)) { | ||
| throw new TypeError( | ||
| "Failed to execute 'timeout' on 'AbortSignal': The provided value have to be a non-negative number.", |
| */ | ||
| static any(signals: AbortSignal[]): AbortSignal { | ||
| if (!Array.isArray(signals)) { | ||
| throw new Error('The signals value must be an instance of Array'); |
There was a problem hiding this comment.
this should be a TypeError
| } | ||
|
|
||
| const listeners = new WeakMap<AbortSignal, () => void>(); | ||
| Object.defineProperty(AbortSignal.prototype, `onabort`, { |
There was a problem hiding this comment.
Please check "EventHandlerAttributes" to implement this.
There was a problem hiding this comment.
Cool, much simpler way to define a onabort
| }); | ||
|
|
||
| // `toString()` should return `"[object AbortSignal]"` | ||
| if (typeof Symbol === 'function' && typeof Symbol.toStringTag === 'symbol') { |
There was a problem hiding this comment.
Yeah, this check is unnecessary
| @@ -0,0 +1,69 @@ | |||
| /** | |||
| * @flow strict | |||
There was a problem hiding this comment.
We should have a proper license here. Given that you copied the implementation from an external package, you should keep the original license in the header for this file. Something like:
/**
* Based on abort-controller by Toru Nagashima
* https://github.com/mysticatea/abort-controller
*
* Original work Copyright (c) 2017 Toru Nagashima
* Modified work Copyright (c) Meta Platforms, Inc. and affiliates.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*
* @flow strict
* @format
*/
| /** | ||
| * Returns the `AbortSignal` object associated with this object. | ||
| */ | ||
| // $FlowExpectedError[unsafe-getters-setters] |
There was a problem hiding this comment.
Could you please disable the lint instead at the top of the file via // flowlint unsafe-getters-setters:off, like we do in
| */ | ||
| constructor() { | ||
| super(); | ||
| throw new TypeError('AbortSignal cannot be constructed directly'); |
There was a problem hiding this comment.
Could you please use the same pattern that we use in other DOM classes in RN to prevent them to be instantiated externally while being allowed internally? That would avoid the Flow issues in other parts of the code.
There was a problem hiding this comment.
Could you please give an example ?
There was a problem hiding this comment.
| * Create an AbortSignal object. | ||
| */ | ||
| export function createAbortSignal(): AbortSignal { | ||
| const signal = Object.create(AbortSignal.prototype); |
There was a problem hiding this comment.
Please use const signal = new AbortSignal(). See my comment above about the pattern of private constructors.
|
|
||
| abortedFlags.set(signal, true); | ||
| reasons.set(signal, reason); | ||
| // $FlowExpectedError[incompatible-type] |
There was a problem hiding this comment.
Why is this Flow suppression necessary?
There was a problem hiding this comment.
doesn't need anymore, removed
c64a914 to
14ae1b0
Compare
|
@rubennorte All comments were fixed please review again |
14ae1b0 to
d7756d6
Compare
|
|
||
| import DOMException from '../../../errors/DOMException'; | ||
| import {AbortController} from '../AbortController'; | ||
| import {AbortSignal_public as AbortSignal} from '../AbortSignal'; |
There was a problem hiding this comment.
tests based on AbortSignal_public class
| () => require('abort-controller/dist/abort-controller').AbortSignal, // flowlint-line untyped-import:off | ||
| () => | ||
| require('../../src/private/webapis/dom/abort-api/AbortSignal') | ||
| .AbortSignal_public, // flowlint-line untyped-import:off |
There was a problem hiding this comment.
RN uses a public interface , same as tests do
https://github.com/react/react-native/pull/57230/changes#r3491521247
| // Copy static properties so that callers accessing them via the public constructor (e.g. `AbortSignal.timeout(0)`) still work. | ||
| // $FlowFixMe[unsafe-object-assign] | ||
| // $FlowFixMe[not-an-object] | ||
| ['timeout', 'abort', 'any'].forEach(methodName => { |
There was a problem hiding this comment.
Might be better to use Object. getOwnPropertyNames() here
There was a problem hiding this comment.
sure, code looks clear now
Object. getOwnPropertyNames(AbortSignal) // ['length', 'name', 'prototype', 'abort', 'any', 'timeout']
| }); | ||
| }); | ||
|
|
||
| describe("'any' static method", () => { |
There was a problem hiding this comment.
Could you please also add tests for the static abort method?
There was a problem hiding this comment.
yes, tests were added
| } | ||
|
|
||
| /** | ||
| * AbortSignal cannot be constructed directly. |
There was a problem hiding this comment.
This comment is outdated. We should indicate this is the internal/private constructor
d7756d6 to
ed56991
Compare
|
|
||
| constructor() { | ||
| super(); | ||
| abortedFlags.set(this, false); |
There was a problem hiding this comment.
Might just be easier to convert this into a WeakSet and call it abortedSignals.
Edit: actually, why don't we store this state in a private field in the signal itself? I think it'd be more efficient. Same for reasons.
There was a problem hiding this comment.
Edit: actually, why don't we store this state in a private field in the signal itself? I think it'd be more efficient.
yes we can use private fields, I tried to keep this fork close to the orignal imlp. to avoid any unexpected breaking changes. so 7 years ago there wasn't any private fields and WeakMap works as alternative to make a private field.
Let me know if you agree to convert it to private fields
There was a problem hiding this comment.
Yeah, let's convert them
There was a problem hiding this comment.
@rubennorte actually, I have no idea how to abort a signal from an AbortController::abort() method
without exposing a public method to mutate that this.#aborted filed
There was a problem hiding this comment.
Oh, that's fair. You can use a symbol property instead of a private method, the same way we did in EventTarget. Check EventInternals. As an added benefit, that method is actually faster than private properties atm.
|
Sorry I missed some details in the original review about the weakmaps for signal state. Can you take a look please? |
8c8e3e5 to
d754299
Compare
…AbortSignal.timeout(time)`
d754299 to
b3594af
Compare
|
@rubennorte would you mind to review it again ? |
| signal[ABORTED_KEY] = true; | ||
| // $FlowExpectedError[prop-missing] | ||
| signal[REASON_KEY] = reason; | ||
| signal.dispatchEvent(new Event('abort')); |
There was a problem hiding this comment.
This should be a trusted event, so it should be dispatched via dispatchTrustedEvent (see EventTargetInternals).
rubennorte
left a comment
There was a problem hiding this comment.
There's just one detail that I can fix on my end. Thanks for the contribution and iterating on this, @retyui !
Summary:
Issue: #55247
We had a similar situation with
promisepackage that wasn't maintained for long time, so Hermes team simply forked it https://github.com/facebook/hermes/blob/adf76f44a5175455a43c984e7d792015ebc3be2a/lib/InternalJavaScript/01-Promise.js#L53 to fix impl. issues and add more featuresChanges:
abort-controller(andevent-target-shimas subdep.) packagesabort-controller/srcfiles to thepackages/react-native/src/private/webapis/dom/abort-api/folderAbortSignal.abort(reason),AbortSignal.any(signals),AbortSignal.timeout(time),signal.throwIfAborted();event-target-shimhas duplicate logic frompackages/react-native/src/private/webapis/dom/events/EventTarget.jsChangelog:
[GENERAL] [CHANGED] - Replace
abort-controllerwith fork version from react-nativeTest Plan: