search

area17 / blast

Storybook for Laravel Blade 🚀
https://dev.to/area17/getting-started-with-blast-storybook-for-laravel-blade-c5c
Apache License 2.0
270 stars 39 forks source link

Use base_path() for default data_path #69

Closed Orclyx closed 1 year ago

Orclyx commented 1 year ago

This wraps the default value used for data_path with base_path(), which provides similar handling to what is being done in getVendorPath() already.

The relative data_path was causing an issue where when running a Laravel worker process, the relative path seemed to be relative to somewhere other than the base path and the worker didn't have permission to write there. This caused a fatal error similar to the one described in https://github.com/area17/blast/issues/61

mrtimbrook commented 1 year ago

Good catch! Thanks for adding this.

I was just testing it and getting an error when including the data files as it adds the base_path again to each file on line 56 (https://github.com/area17/blast/blob/1.x/src/DataStore.php#L56).

If you could remove the base_path here as part of this PR we should be good to merge. Thanks!

Orclyx commented 1 year ago

@mrtimbrook Ah have changed, thanks.

It occurred to me that this isn't as close to how getVendorPath() works as maybe it ought to be: if you set a relative vendor_path in the blast.php config file then getVendorPath will apply base_path(), but it won't if vendor_path is absolute (good). If you set a relative base_path in config that probably won't work any more which seems bad. Happy to iterate on this further to solve if you'd like.

mrtimbrook commented 1 year ago

I think you're right and it makes sense to follow the same pattern as the getVendorPath() method.

Orclyx commented 1 year ago

Have reworked and added tests for the different scenarios:

mrtimbrook commented 1 year ago

Sorry for the delayed reply. This all looks great (the tests are 🔥 )! Thanks for your work.