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

First class TypeScript support #655

Merged
merged 5 commits into from
Oct 6, 2023
Merged

Conversation

lmeysel
Copy link

@lmeysel lmeysel commented Jul 28, 2023

I pretty much appreciate your library but everytime I use it, I wonder if it would be possible to have it integrated into modern tooling. There are several issues regarding typescript support, but honestly, most of them are not very satisfying and the @types/zigg-js package also does not integrate well in my environments.

So I started an experiment which utilizes typescript's interface Augementation to "polyfill" the generated routes and migrated the whole JS code to TypeScript.
For better integration I also introduced a modified version of the vite-plugin from #321 (comment).

As a result the new ziggy could come with out-of-the-box TS support and vite integration.

I hope you like it and you are interested in discussing this!

Highlights

Vite Integration

Use ziggy in vite and generate route-declarations everytime the route-files change:

import { defineConfig } from 'vite';
import Ziggy from 'ziggy-js/vite';

defineConfig({
  plugins: [
    Ziggy({ declarations: 'only' }),
  ]
});

TS Integration

// routes/web.php
Route::get('/users/{user}/posts/{post}/comments', [CommentsController::class, 'index'])->name('users.posts.comments');

Now VSCode will support you like this:
image

... and, much better, like this:
image
image

@bakerkretzmar
Copy link
Collaborator

@lmeysel this is awesome, thank you so much! Lots to go over here, so it's gonna take me a while. I love the autocompletion, I've been trying to figure out how I want that to work for ages. I have a local branch where I've rewritten Ziggy in TypeScript too, and some of the code I've written is strikingly similar to what you have here, which I think is a great sign.

One initial question: do you think it's possible to get all the autocompletion and intellisense benefits of this PR without converting Ziggy itself to TypeScript at the same time?

@lmeysel
Copy link
Author

lmeysel commented Aug 15, 2023

Actually this was my first attempt. I started from the @types-package but I often found it to be unreliable. I guess it is possible to start over without converting the package to typescript itself, but then I would at least make sure to ship the types with the package (and keep them up to date).
Would you like to release a TS version e.g. with the next major?

@bakerkretzmar
Copy link
Collaborator

Interesting. Yeah that package is out of date. I don't really have any particular goal around releasing a TS version, it could definitely happen before the next major version I just don't want it to hold up other things, and I want to be really thorough to make sure it's as good as I want it to be and provides enough benefit to be worth it.

@bakerkretzmar bakerkretzmar self-assigned this Aug 18, 2023
@lmeysel
Copy link
Author

lmeysel commented Aug 23, 2023

Okay, so how should we proceed? Do I understand you correct, you want to keep ziggy in JS in general? Alternatively, it might be a worth thinking of releasing the rewritten ziggy as new major in alpha state and then wait for community feedback

@bakerkretzmar
Copy link
Collaborator

@lmeysel I think that's what I would like to try to do, yeah. I think releasing the typescript rewrite at the same time as v2 makes sense, it's a logical place to make some other changes I've been wanting to for a long time (only shipping ES modules, renaming exports, etc.).

I'm going to keep exploring and reviewing all your code here and I might leave questions on this PR. I want to really understand how all of this fits together so it may take a while, I appreciate your patience. I think the best next step would be to get a PR together with only the bare minimum functionality required for the autocompletion demonstrated in this PR, which I'm guessing means all the PHP .d.ts file generation and a much smaller types.d.ts or similar file that we ship? For now I want to hold off on the Vite plugin and the complete TypeScript rewrite (very interested in both but I want to consider them all separately).

If you want to get that PR going go for it, otherwise I will, based on this one. I'm happy to do the work now to tweak this and pull out only what's necessary but I want to make sure you get the credit 😄

Thanks again, this is really exciting!

@bakerkretzmar
Copy link
Collaborator

@lmeysel can you also push up or share some examples of the generated ziggy.d.ts files? I'd like to include them in the tests as fixture files but the generated ones I get locally running your tests are all empty. Thanks!

@lmeysel
Copy link
Author

lmeysel commented Sep 4, 2023

Hi, this are the definitions from tests/Unit/RouteModelBindingTest.php. Does this work for you?

print_r((new Ziggy())->typescriptDeclarationGenerator()->generateDeclarations());
// ziggy.d.ts

/*
 * Do not modify this file. It is auto-generated and corresponds to your routes
 * exposed by ziggy route helper. Changes will not be preserved.
 */
export {}

declare module 'ziggy-js' {
  interface RouteLookup {
    'users': [{ name: 'user', binding: 'uuid' }],
    'admins': [{ name: 'admin', binding: 'uuid' }],
    'tags': [{ name: 'tag', binding: 'id' }],
    'tokens': [{ name: 'token' }],
    'users.numbers': [{ name: 'user', binding: 'uuid' }, { name: 'number' }],
    'users.store': [],
    'comments': [{ name: 'comment', binding: 'uuid' }],
    'replies': [{ name: 'reply', binding: 'uuid' }],
    'posts': [{ name: 'category', binding: 'id' }, { name: 'post', binding: 'slug' }],
    'posts.tags': [{ name: 'category', binding: 'id' }, { name: 'post', binding: 'slug' }, { name: 'tag', binding: 'slug' }],
  }
}

@bakerkretzmar
Copy link
Collaborator

@lmeysel yep that's great, thanks. I might tag you with more questions. Another thing I'm wondering about is whether it would be possible to incorporate this into the existing ziggy.js file with a JSDoc comment. I've seen other tools do this, where you can add a comment with a type declaration to some config object or something and have it autocomplete.

/**
* Holds any known route name or an arbitrary string.
*/
export declare type RouteName = keyof RouteLookup | (string & {});
Copy link
Collaborator

Choose a reason for hiding this comment

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

@lmeysel what does & {} do here? If we want to be permissive and allow strings we don't recognize as route names, why not just keyof RouteLookup | string?

Copy link
Author

@lmeysel lmeysel Sep 19, 2023

Choose a reason for hiding this comment

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

Hmm, not sure if this question is answered in the other PR already: You will need string & {} to make (at least VSCode) to allow strings as well as "specific" strings (i.e. literals). If you just do | string, vscode will make no suggestions, and thus you will not get a list of available routes in intellisense.

Essentially one could say: string swallows literals, but string & {} does not

@bakerkretzmar bakerkretzmar merged commit 42d9248 into tighten:main Oct 6, 2023
0 of 48 checks passed
@bakerkretzmar
Copy link
Collaborator

@lmeysel sorry, didn't mean to close this, I guess GitHub considers it merged since all these commits were included in #664. Thanks again for this PR!!

@slavarazum
Copy link

Any plans to include vite plugin into the bundle? I just have released that it's missing in the main branch.

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