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

fix: unable to evaluate instanceof AjaxError #6275

Merged
merged 1 commit into from Apr 27, 2021

Conversation

voliva
Copy link
Contributor

@voliva voliva commented Apr 27, 2021

Description:

Because the constructor signature of AjaxErrorCtor and AjaxTimeoutErrorCtor are marked as @internal, and #6215 added stripInternals: true, the interface of these two constructors was exported as an empty interface.

Typescript would then complain on projects that use error instanceof AjaxError, as it doesn't identify AjaxError as callable.

Related issue (if exists): #6269

I've found this solution, which I think works nicely: The type is still exported, the constructor signature is still stripped (because it's internal) and typescript now allows to check instanceof AjaxError, while preventing the consumer from trying to call the constructor:

image

@cartant
Copy link
Collaborator

cartant commented Apr 27, 2021

Thanks. You'll need to do npm run api_guardian:update, too.

Ignore that. I'd expected this to remove the @internal, but if the extend Function works, I suppose that's okay. Does it really work?

@voliva
Copy link
Contributor Author

voliva commented Apr 27, 2021

Thanks. You'll need to do npm run api_guardian:update, too.

Ignore that. I'd expected this to remove the @internal, but if the extend Function works, I suppose that's okay. Does it really work?

Mhh now I see that although it does fix the instanceof issue, TS guards fail:

if(error instanceof AjaxError) {
  // typeof error = { }
}

I'll try to give it a bit more thought see if I can keep both (i.e. internal on types, while being able to use typescript guards), otherwise I'll just remove the @internal annotation.

@voliva
Copy link
Contributor Author

voliva commented Apr 27, 2021

by the way, npm run api_guardian:update doesn't seem to have any effect on these interfaces: For some reason they are not getting exposed in the api_guard folder, even when removing @internal

@cartant
Copy link
Collaborator

cartant commented Apr 27, 2021

For some reason they are not getting exposed in the api_guard folder, even when removing @internal

Yeah, you're right. It's only the AjaxError that's exported; the ctor type is not.

@voliva
Copy link
Contributor Author

voliva commented Apr 27, 2021

I just changed to the simple "remove @internal tags" solution.

I did find a solution that would work, but doesn't feel right

export interface AjaxErrorCtor {
  /**
   * Internal use only. Do not manually create instances of this type.
   */
  new (internal: never): AjaxError
  /**
   * @internal
   */
  new (message: string, xhr: XMLHttpRequest, request: AjaxRequest): AjaxError;
}

image

And guards work fine. But I don't fully like it.... also when using it within rxjs you'd also see that fake signature. So I gave up - I removed the internal tag.

@cartant
Copy link
Collaborator

cartant commented Apr 27, 2021

So I gave up - I removed the internal tag.

That's fine. There are a whole bunch of other XErrorCtor interfaces and most don't have an @internal tag.

@benlesh
Copy link
Member

benlesh commented Apr 27, 2021

Why would people need to create their own new AjaxError? This was made internal intentionally, IIRC. It's fine for instanceof checks. I wish I could stop inheritance... But I can't see a reason anyone should be creating new AjaxError, unless I'm missing a use case?

@benlesh
Copy link
Member

benlesh commented Apr 27, 2021

To be clear: I'm not blocking this PR. I'm just saying I'm not sure we want to expose the ability to create new AjaxErrors, and I'm totally willing to change my mind.

@benlesh
Copy link
Member

benlesh commented Apr 27, 2021

Nevermind, I need to learn to read more thoroughly:

Typescript would then complain on projects that use error instanceof AjaxError, as it doesn't identify AjaxError as callable.

@benlesh benlesh merged commit a7c2d29 into ReactiveX:master Apr 27, 2021
@voliva voliva deleted the internal-ajax-error branch April 28, 2021 20:14
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.

None yet

3 participants