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

Fix assets being generated in current working directory #290

Merged
merged 6 commits into from May 8, 2020

Conversation

bakerkretzmar
Copy link
Collaborator

@bakerkretzmar bakerkretzmar commented May 1, 2020

This PR uses Laravel's base_path() helper to ensure that the ziggy:generate command always outputs assets to the correct location, even when run somewhere other than the project root.

I added a test to ensure this behaves correctly. In order for the test to work, and because Ziggy will now always use an absolute file path internally, I updated the tests to use the real filesystem instead of a virtual one. The files are generated inside Testbench's dummy Laravel app, at vendor/orchestra/testbench-core/laravel/..., and the new tearDown() method ensures they're deleted between tests. The vfsstream dependency was no longer needed, so I removed it.

Fixes #282.

@bakerkretzmar bakerkretzmar merged commit 292a6eb into master May 8, 2020
@bakerkretzmar bakerkretzmar deleted the jbk/fix-generated-routejs-path branch May 8, 2020 12:16
@rodrigopedra
Copy link
Contributor

rodrigopedra commented May 13, 2020

I know this PR was for good, but it broke my setup and I spent some time trying to figure out what happened.

With previous ziggy versions I was using the resource_path helper in my config/ziggy.php.

So after the last update ziggy exported its js file to something like this:

/home/rodrigo/code/project/home/rodrigo/code/project/resources/js/ziggy.js

Took me a while to find the related release note. But now I have it fixed.

Maybe it is worth to add a note about this behavior change on the readme, in case someone else were already using an absolute path like me.

Please don't take as a rant, I really like the lib and appreciate all the hard work and effort you put in it. Thanks a lot.

@bakerkretzmar
Copy link
Collaborator Author

@rodrigopedra thanks a lot for letting us know! Sorry for the confusion, I'll come back and try to make a note of this in the Readme for other people who might run into it.

I'm curious about how this happened, can you share that section of your config/ziggy.php? I didn't think we had a config option for the output path.

@rodrigopedra
Copy link
Contributor

Indeed ziggy did not use that configuration You're correct, I apologize for the false reporting.

My confusion was due that for a particular project I need to export group of routes in different paths (public routes and admin panel routes).

To ease that I wrote a custom command that calls ziggy's own command underneath and for convenience I stored those paths in the same ziggy config file.

After updating it broke my frontend pipeline that called my command instead of ziggy's.

So never mind and sorry for any inconvenience. It was a total confusion on my side.

Thanks a lot for the great package and for the quick response.

For reference below is my custom command:

<?php

namespace App\Console\Commands;

use Illuminate\Console\Command;
use Illuminate\Contracts\Config\Repository;

class GenerateZiggyRoutes extends Command
{
    protected $signature = 'app:ziggy';

    public function handle(Repository $config)
    {
        $paths = $config->get('ziggy.paths', []);

        foreach ($paths as $group => $path) {
            $this->call('ziggy:generate', [
                'path' => $path,
                '--group' => $group,
            ]);

            $this->info("[ZIGGY] {$group} routes generated");
        }
    }
}

And my config file:

<?php

return [
    'blacklist' => ['debugbar.*', 'horizon.*'],

    'paths' => [
        'admin' => 'resources/assets/js/_admin/ziggy.js',
        'app' => 'resources/assets/js/_app/ziggy.js',
    ],

    'groups' => [
        'admin' => [
            'home',
            'admin.*',
        ],

        'app' => [
            'home',
            'app.*',
        ],
    ],
];

Maybe it would be a good idea to add some similar feature to ziggy itself. Let me know if you think this is a good idea and I can try sending a PR.

@bakerkretzmar
Copy link
Collaborator Author

Thanks a lot for sharing that command and config, that's really cool. I can definitely see the use case for this—we already more or less support it via the @routes directive, it accepts a group parameter so you can load different groups on different pages.

Feels very related to #272 and likely wouldn't be tough to implement. If you have time to PR this please do!

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.

Routes js file path is relative to current dir rather than project root
2 participants