Specify ErrorResponse as interface to provide obvious contract

[sgrishchenko]

There is problem in current declaration of ErrorResponse: the private field leaks to the public contract. So ErrorResponse type behaves very strange on usage (see playground).
I propose to simplify the declaration of ErrorResponse and declare it's public interface obviously.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6a0543f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages

Name	Type
@remix-run/router	Patch
react-router	Patch
react-router-dom	Patch
react-router-dom-v5-compat	Patch
react-router-native	Patch

Not sure what this means? Click here to learn what changesets are.

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

[remix-cla-bot]

Hi @sgrishchenko,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[brophdawg11]

Can you explain the use-case for which you are trying to create an ErrorResponse? These are intended to only ever be created by the framework and exposed via useRouteError and then type-narrowed in user-land via isRouteErrorRepsonse:

https://www.typescriptlang.org/play?noUncheckedIndexedAccess=true&exactOptionalPropertyTypes=true&ts=5.1.6#code/JYWwDg9gTgLgBAb2AZwEoQK4wKYFEpTSrbKQB2y2ANHPoVMaRBdXAKoByAygIIBiuAPp0iJcpQCS4ADYBfOADNCIOACIAAlGwhgADwC0UDGQD0hLNiiqAULZMm4AeTLSAnnBgALbHGBkcUGQAhtLIcBAKHt6KUEEg2ADu0ADWcMiemNIAJnAARj4AxlpBMH4A5lEk2NbS2PBaTCxwALxwZIns3PxCIgxizJIyABQALAAMYzQA5LlBOVoAjhgkMFM0CGkQ8QBccFNZJUFTcLIAlADcdg4SZHAYlFD60kFkWTSumGkZGNl5PjCuMDYfTBAgQBLlazASJDFDoCy9RjibBDBrI06nRDWOBwaQQMq9VH9FgXayyK5wHivO7IcqVOBgDC5aTAAoeQE+IJhdrYLK86wKYwFUrMXH4wnYXaI4mUTEIbFwAoDCC1AB0eLKQ2wquQMBK9xo2t1+uQABVsLoYIbVQc9aSFfY4AAFKDAABuJR8CmA2F+WQgJDaEHgtSCyQV2r8AWC0jJQA

[sgrishchenko]

The first point is that with my changes, you don't need 3-line comment about interface/implementation pattern. Now code is self-documented.

The second point: in the comment I removed there was the statement:

but we do want to export the shape so folks can build their own abstractions around instances

And your example perfectly demonstrates that with logError function. But what if I want to write unit test for logError? I will end up with force type casting or something like that:

logError({
  status: 400, 
  statusText: 'bad request',
  data: undefined,
} as ErrorResponse)

https://www.typescriptlang.org/play?noUncheckedIndexedAccess=true&exactOptionalPropertyTypes=true&ts=5.1.6#code/JYWwDg9gTgLgBAb2AZwEoQK4wKYFEpTSrbKQB2y2ANHPoVMaRBdXAKoByAygIIBiuAPp0iJcpQCS4ADYBfOADNCIOACIAAlGwhgADwC0UDGQD0hLNiiqAULZMm4AeTLSAnnBgALbHGBkcUGQAhtLIcBAKHt6KUEEg2ADu0ADWcMiemNIAJnAARj4AxlpBMH4A5lEk2NbS2PBaTCxwALxwZIns3PxCIgxizJIyABQALAAMYzQA5LlBOVoAjhgkMFM0CGkQ8QBccFNZJUFTcLIAlADcdg4SZHAYlFD60kFkWTSumGkZGNl5PjCuMDYfTBAgQBLlazASJDFDoCy9RjibBDBrI06nRDWOBwaQQMq9VH9FgXayyK5wHivO7IcqVOBgDC5aTAAoeQE+IJhdrYLK86wKYwFUrMXH4wnYXaI4mUTEIbFwAoDCC1AB0eLKQ2wquQMBK9xo2t1+uQABVsLoYIbVQc9aSFfY4AAFKDAABuJR8CmA2F+WQgJDaEHgtSCyQV2r8AWC0jJFIkcASL3gMAgcBAYf+nhQipC0l8t2MwBTKzgNXFYKgQ3lOONMHuu3GkzLtb19bNFpguxmczgi2WurWCttQV2xj53vabzJpwpACEsHAEwUXlMrZsl3BPEE3f800CoApoPF2UDc7rIRrCTW0m2G3AmzQFXX7ubLd3ZvNsEsVkOcSOx1ebBJ15KgyTgLlaErJEBmwU4gA

And the third point: with the current declaration, I don't see the public contract of ErrorResponse and what data I can use from it. To get this information, I need to investigate the code from internal implementation of UNSAFE_ErrorResponseImpl and I think it is conceptually wrong.

[brophdawg11]

Thank you - the unit test is a great use-case for why we should make the change. I was just concerned that this was something being used incorrectly in app-code 👍

you don't need 3-line comment about interface/implementation pattern. Now code is self-documented.

Eh - "Self documenting code" is a buzzword I've seen abused too much over the years. Code describes "what"/"how", comments describe "why". Removing this comment is detrimental because we lose the reason for why we keep the interface and class separate in the first place.

[brophdawg11]

@sgrishchenko Do you mind rebasing this and repointing it to dev since it contains code changes?

[sgrishchenko]

Ok, probably I'm wrong with the point that the comment is unnecessary anymore. I returned the comment back.
Of course, I will rebase my branch, no problem.
What do you mean by "reporting to dev"? I never did it before. Is there some guide for it or something like that?

[sgrishchenko]

Oh, you mean dev branch? if so, I will do it, no problem.

[brophdawg11]

Can we keep this as a type to avoid changing the semantics to interface?

[sgrishchenko]

Sure, we can, but from my point of view it exposes the public API, so it is exactly an interface semantically, or am I missing something? Maybe you want to avoid interface merging?

[brophdawg11]

yeah, we prefer type over interface unless we need the ability for end users to augment the interface, which we shouldn't in this case

[sgrishchenko]

Done

[github-actions]

🤖 Hello there,

We just published version 6.17.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[github-actions]

🤖 Hello there,

We just published version 6.17.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!