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

Auto-generate shield() calls using template literal types, #23 #25

Merged
merged 23 commits into from
Apr 22, 2022

Conversation

louwers
Copy link
Collaborator

@louwers louwers commented Apr 18, 2022

As discussed, a Draft MR for #23 👍

Action points:

  • Getting the contents of types.ts for the test and inside the transformer in a clean way (i.e. without using __dirname). (See FIXME inside transform.ts and transformer.spec.ts).
  • I realized that I didn't consider optional tuple elements, so they are currently broken. This is a problem for optional function arguments:
export function doSomething(arg: string, arg2?: string)
  • Another case I realized is tuples which contain undefined or null. [string, null]. Right now this is unrepresentable with shield(). Not such a big problem because who would want this. But it should just fail to generate a shield() call at all if someone tries.

@louwers louwers changed the title Auto-generate shield() calls using template literal types, #23 Auto-generate shield() calls using template literal types, #23 Apr 18, 2022
@brillout
Copy link
Owner

Performance shouldn't be a problem, but let's have an idea of how long the transpilation is delayed by the new transformer.

Worst case we only apply the transformer for prod building. Since the user's IDE already does the type checking job in dev. We can always make this an option.

@louwers
Copy link
Collaborator Author

louwers commented Apr 19, 2022

@brillout W.r.t performance I generated this file: https://gist.github.com/louwers/392db2e700f9f597d8eecc24b7836c1f

Good news, transformer disabled:

real    0m0,881s
user    0m0,806s
sys     0m0,085s

Enabled:

real    0m2,883s
user    0m4,400s
sys     0m0,159s

Which is very good for such a complex types and so many functions. So I'd say we just keep it enabled for dev mode, but monitor real-world performance.

Bad news: I found that the diagnostics I use for extracting the value of template literal types gets cut off:

Encountered unrecognized emit message:  Type '"fun94"' is not assignable to type '"[
t.union(t.const(94), { geometries:t.array(t.union({ coordinates: t.array(t.array(t.array(
t.array(t.number)))), type: t.const('MultiPolygon') }, { coordinates: t.array(t.array(t.array(
t.number))), type: t.const('Polygon') }, { type: t.const('MultiLineString'), coordinates:
t.array(t.array(t.array(t.number))) }, { ...'.

so I have to find a better approach. The author of ts-morph is likely on it as well: dsherret/ts-morph#1266

@brillout
Copy link
Owner

I'd say we just keep it enabled for dev mode, but monitor real-world performance.

👍 We may want to have development experience that is as fast as possible development, so yea let's monitor on real world apps.

Bad news: I found that the diagnostics I use for extracting the value of template literal types gets cut off:

Seems like it would happen only rarely and, more importantly, deterministically though.

@brillout
Copy link
Owner

Btw. a potential solution for the problem of the emit info being truncated:

I can see that the threshold for truncating to be some const variable in TypeScript.

If that's the case then it's easy to change it to some big number 9999 in a post-install script. (I believe TypeScript ships as JavaScript and not binary; should be easy to patch TypeScript.)

We pin TypeScript and check if the patch still works when we use a newer TypeScript version.

@louwers
Copy link
Collaborator Author

louwers commented Apr 21, 2022

Btw. a potential solution for the problem of the emit info being truncated:

I can see that the threshold for truncating to be some const variable in TypeScript.

If that's the case then it's easy to change it to some big number 9999 in a post-install script. (I believe TypeScript ships as JavaScript and not binary; should be easy to patch TypeScript.)

We pin TypeScript and check if the patch still works when we use a newer TypeScript version.

I'm using .getLiteralValue() in a way that seems to work consistently now!

assertWarning(false, `Failed to generate shield() for telefunction '${teleFunName}'`)
continue
}
const shieldStrWithAlias = shieldStr.replace(/t\./g, `${tAlias}.`)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function hello(name: 'some-t.string') {
}

I believe this would break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

@brillout
Copy link
Owner

Let me know when the problem function hello(name: 'some-t.string') is solved. I'll then merge. FYI: I renamed variables to be consistent with the rest of Telefunc's source, and added an e2e test to examples/vite/.

@louwers
Copy link
Collaborator Author

louwers commented Apr 22, 2022

Let me know when the problem function hello(name: 'some-t.string') is solved. I'll then merge. FYI: I renamed variables to be consistent with the rest of Telefunc's source, and added an e2e test to examples/vite/.

Awesome! 🎉 I'll let you know.

@louwers
Copy link
Collaborator Author

louwers commented Apr 22, 2022

Made an additional fix to make sure single quotation marks are escaped.

@brillout brillout merged commit 9979b44 into brillout:master Apr 22, 2022
@brillout
Copy link
Owner

Released in 0.1.20.

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.

3 participants