tighten / ziggy

Use your Laravel routes in JavaScript.
MIT License
3.86k stars 247 forks source link

Improvements on ziggy.d.ts generation #680

Open lmeysel opened 11 months ago

lmeysel commented 11 months ago

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)

bakerkretzmar commented 10 months ago

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 import routeFn from 'ziggy-js';. If we have to generate this declaration automatically we can't necessarily know that ziggy-js exists to import from—the user might not have the NPM package installed and might not have a bundler/TS alias set up to point ziggy-js to their vendor folder. (Actually, is that true? Or to get correct types without the NPM package would they have to set up the path alias in their tsconfig, and therefore it would work fine?) That means we'd have to import from the vendor directory ourselves using a relative file path, which means we'll have to generate that path based on where the file is generated... which feels gross. Am I missing anything? I'll play around with this some more and see if I can answer some of these questions myself.

lmeysel commented 10 months ago

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)