-
Notifications
You must be signed in to change notification settings - Fork 253
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
Improvements on ziggy.d.ts generation #680
Comments
That file path convenience makes sense 👍🏻 The declarations I'm interested in but I have some questions. Just thinking out loud: In the example in the Readme we |
I'll read this again tomorrow, but if I understand correctly: When generating the ziggy.d.ts you definitely will have the composer package installed. So from my perspective there is nothing wrong referencing the types from within the composer package (as you suggest to do in the tsconfig.json otherwise). Maybe it is a good idea to give a bit more control about this to the user, e.g. like passing a flag (--ziggy-types=node|php). Alternatively (or additionally) you may inspect the package.json in CWD in order to find out there is the ziggy-js package installed. Or check the node_modules directory directly. When checking node_modules folder be careful of how you do this check, since diffrent package managers do different things (e.g. like not placing a folder but a symlink) |
I tried the newly introduced TS support and it works great so far.
I would like to discuss two further improvements (it is just cosmetics):
When generating the .d.ts file using
php artisan ziggy:generate <path>
, then<path>
must point to a JS file. This made sense before the TS introduction, but when using--types-only
flag, that is a bit inconvinient. Instead I suggest to allow arbitrary files and change the extension if it is not already .js/.d.ts respectively.Second: In the readme, you ask to add another .d.ts file to basically shim the
routeFn
and make it globally available for TS. Since we also need generating the declarations for the route, it probably make sense, to add the shim to the definition (or at least having another flag in the command which creates the shim right along in the ziggy.d.ts)The text was updated successfully, but these errors were encountered: