-
Notifications
You must be signed in to change notification settings - Fork 20
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
[WIP] Full Stack Type Safety #58
Draft
Etheryte
wants to merge
3
commits into
uyuni-project:master
Choose a base branch
from
Etheryte:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
- Feature Name: Full Stack Type Safety | ||
- Start Date: 2021-05-28 | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Add type safety to frontend and backend communications to achieve type safety across the whole project. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
With the wrapup of the Typescript migration, we now have type safety both in the backend project by itself and in the frontend project by itself. What we do not have is type safety in the communications between the two. If an entity changes on the backend but the corresponding code is not updated on the frontend, the regression can go unnoticed unless this specific interaction is covered by an E2E test. Adding type checks to backend and frontend communications will catch these regressions. In addition, shared types will also help write sturdier frontend code since it will alert the developer to possible nullable values and other similar edge cases which might otherwise go unnoticed. | ||
|
||
# Detailed design | ||
[design]: #detailed-design | ||
|
||
In the order of increased difficulty but also increased payoff, there's two possible approaches. The easier but more laborious option is to generate a set of exposed types and manually annotate requests. | ||
|
||
```ts | ||
// Type definitions generated from the backend project... | ||
namespace Space { | ||
export interface Foo { | ||
goodExample: boolean; | ||
} | ||
} | ||
``` | ||
|
||
```ts | ||
// ...can be used to manually annotate expected types in the frontend project. | ||
- const result = await Network.get("/rhn/manager/getFoo"); | ||
+ const result = await Network.get<Space.Foo>("/rhn/manager/getFoo"); | ||
|
||
// 'goodExample' is correctly identified as a boolean value | ||
if (result.goodExample) { | ||
// ... | ||
} | ||
|
||
// Throws an error whereas previously this passed silently | ||
if (result.badExample) { | ||
// ^^^^^^^^^^ | ||
// Property 'badExample' does not exist on type 'Foo' | ||
} | ||
``` | ||
|
||
The general workflow for using this approach would be to add a step to the backend build process which generates a type declaration file as shown above. There are existing tools which provide this functionality, I've considered evaluating them out of scope for the time being. The generated file can be added to the frontend project scope and the type definitions can be used accordingly where needed. This change can be adopted gradually over time. | ||
|
||
An alternative option would be to try to automatically annotate requests. It's unclear whether this is realistically implementable, but I've included it for pink dreams' and completeness sake. | ||
|
||
This relies on a [suggested Typescript feature](https://github.com/microsoft/TypeScript/issues/41160) that's not approved and might not be approved. In addition to the Typescript limitation, it would also be necessary to find a way to map request URLs to corresponding types which might not be technically realistic. | ||
|
||
```ts | ||
// Types generated similar to the above option | ||
namespace Space { | ||
export interface Foo { /* ... */ } | ||
export interface Bar { /* ... */ } | ||
} | ||
``` | ||
|
||
```ts | ||
// Generated overload definitions | ||
function get(url: "/rhn/manager/getFoo"): Promise<Space.Foo>; | ||
function get(url: "/rhn/manager/${^[a-zA-Z0-9]*$}/getBar"): Promise<Space.Bar>; | ||
function get(url: string) { | ||
// Implementation omitted | ||
} | ||
``` | ||
|
||
```ts | ||
// 'result' is automatically inferred as 'Space.Bar' | ||
const result = await Network.get("/rhn/manager/1234/getBar"); | ||
``` | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
Given how Java handles `null`, it may be infeasible to try export types that realistically reflect the expected entity state. Optionally chaining every field in a nested object and adding fallbacks where we know values to actually be present would quickly outweigh benefits we might otherwise get from type safety simply by adding a lot of overhead and additional complexity. This is mostly a tooling question and I don't know what the current state of the art is in this regard. | ||
|
||
Depending on the volume of types that will be exposed, the time to export those types as well as the time to then build the frontend project with them may be too large. Depending on the tooling, hot reload performance may take a considerable hit, slowing down development. Likewise, if those types are in scope for the IDE autocomplete, past some large number of types it might start slowing down opening the project, using autocomplete, etc. I don't have an estimate for what order of magnitude would become problematic. | ||
|
||
If type annotation can't be automated, finding the correct types manually would become a chore, even if adopted gradually. Manual type annotations are likewise susceptible to rot over time especially in cases where instead of an interface changing, a new one is used entirely. If the types overlap initially, the change may go unnoticed until some later time when they diverge. | ||
|
||
Implementing automatic annotations based on overloads or similar may be infeasible for performance reasons, this is not something I have seen another project implement at this scale before, in large part because Typescript added some of the tools to make this possible only fairly recently. | ||
|
||
Type names will need to be consistent and uniquely identifiable. Depending on the tooling we use this may pose considerable challenges. Based on the use case, similar entity names crop up in many different contexts and disambiguating them may be difficult or downright impossible without renaming one or the other if the tooling doesn't specifically consider this. Scoping and other similar solutions can help here, but this is largely implementation specific. | ||
|
||
Implementing type safety across the full stack can make quick prototyping harder. Depending on the implementation and the developer's workflow, having the frontend build error out on backend changes may hamper fast iterative development. This can be overcome by manually annotating the type to be `any` on the frontend side while prototyping. | ||
|
||
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
The easiest alternative is to keep things as they are and fix any related bugs as they come up. Depending on the implementation, the effort to reach full stack type safety can be considerable, and it may be easier to reactively fix bugs rather than proactively try to avoid them. | ||
|
||
It is also possible to manually write the types we expect to receive and use them today. Those are highly susceptible to rot though, since a developer making a change in the backend code may not be aware of these annotations in the frontend or might not find them. | ||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
It is unknown whether any currently existing tool for type generation matches all of the requirements outlined in this document. | ||
|
||
The automated type annotation proposal relies on a [suggested Typescript feature](https://github.com/microsoft/TypeScript/issues/41160) that's currently being discussed, hasn't been approved and might not be approved. Without this feature, automated annotations may not be feasible even if the URL to type mapping is possible. Whether this feature will ever be implemented and in what timeframe is unknown. | ||
|
||
The performance impact on the IDE, the change in build times, and other similar overheads are all unknown for a project of this size. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 am not against the overall idea, but think it's critical to define how this type declaration file can be generated from the backend code.
Without that piece, I guess the only possibility would be to create it manually. But in my understanding, that would only move the problem one step away, from "backend to frontend" type coherence to "backend to declararation-file" type coherence - correct?
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.
That is correct, yes. Without automated tooling, I don't think this proposal is reasonable. I haven't had the chance to see this approach in action on a Java project before so I don't know what tools may be applicable or realistically pluggable with our architecture. At my previous job we had a good run using NSwag on a .NET Core project, the flow there was that our backend project had Swagger tacked on and NSwag used the JSON output by Swagger to generate Typescript types.
NSwag did have one major drawback in that the generated type names were not consistently unique. You could have entities with the same name in different business scopes, a-la both named
Entity
, mapped to names such asEntity1
andEntity2
in the output but whenEntity1
was removed in a refactor,Entity2
would then become justEntity
. That was a big pain point for us, I haven't checked whether they've found a fix for it since.