tighten / collect

A Collections-only split from Laravel's Illuminate Support
1.51k stars 110 forks source link

Symfony dump() dependency. #199

Closed mittelgrau closed 2 years ago

mittelgrau commented 4 years ago

As far as i can see the Symfony\VarDumper Dependency is just used once in a helpers method. Is it essential? A simple global non namespaced helper function with a pretty unspecific name like dump() command often has the potential to cause conflicts with other utility methods. Would be of great help if a native solution could be used. Not really a Php guy, are there any critical reasons why the symfony dump is prefered over something like this:

die(var_dump($var));

Thank you for the project!

mattstauffer commented 4 years ago

The main reason is that the output of the two commands is pretty significantly different. The left is the dump() command; the right is your example:

image

Even if you can force the wrapping with <pre>:

image

I'm open to suggestions that don't negatively impact the user experience, if anyone has them!

mittelgrau commented 4 years ago

I see, thank you for taking the time to answer and explaining it.

Th3Mouk commented 4 years ago

Hi @mattstauffer !

What about let the developper choose if he want or not the dependency ? It can be a suggest in composer.json and you can add and assertion on dd() to check if the class is loaded. With a nice message like symfony/var-dump is require to use this method.

And so you can move the dependency to require-dev to ensure the CI still working

mattstauffer commented 4 years ago

@Th3Mouk I love the idea. I'm a little bit not a fan of forcing folks to add another dependency to get something that should feel like it's available out of the box, but to be honest nice messaging telling you exactly what to do to use it isn't all that bad of an idea. Let me think about it!

austinpray commented 4 years ago

Thanks for all the work on this package! I am currently in dependency hell after spamming Illuminate\Support all over my non-laravel codebase.

Honestly didn't expect to see dd() included in this package. Seems out of scope for a "Collections-only split from Laravel's Illuminate Support". Furthermore didn't expect to see symfony/var-dumper as a hard dependency. I got all excited after reading the article in your readme but looks like including symfony/var-dumper violates a lot of the spirit of that article.

In our codebase we have

function pretty_var_dump($something): void {
    if (function_exists('dump')) {
        dump($something);
        return;
    }

    var_dump($something);
}

symfony/var-dumper is required as require-dev. It is not available in a production docker image unless you purposefully composer install in a temporary debug session. Our build is setup to fail if it sees any usage of var_dump(), dump(), dd() left in a pull-request so dd() and such. So dd and var-dumper is strictly a nice-to-have for dev purposes.

Above is basically the setup that https://github.com/tightenco/collect/issues/199#issuecomment-586903347 suggests. I could probably put together a patch that moves symfony/var-dumper to the "suggest" key in composer.json instead of a dependency. Lemme know what you think.

mattstauffer commented 4 years ago

I’m open to a PR