Allow saving to disk #35
Conversation
src/Console/RefreshCommand.php
Outdated
| @@ -42,8 +43,76 @@ public function fire() | |||
| throw new ConfigException('Please set the "locales" config! See https://github.com/andywer/laravel-js-localization#configuration'); | |||
| } | |||
|
|
|||
| MessageCachingService::refreshCache(); | |||
| ConfigCachingService::refreshCache(); | |||
| switch (Config::get('js-localization.storage')) { | |||
There was a problem hiding this comment.
You use the new methods generateMessagesFile and generateConfigFile here to refresh the cache data if Config::get('js-localization.storage') === 'file'. But the generated static files are not used for serving data via the HTTP service (and that would probably introduce a performance penalty).
This seems confusing, since the refresh command is for refreshing the service cache.
|
Hey @Jarlskov! Thanks for your efforts :) I am not sure about the approach of configuring + changing the refresh command (see my comment in the code diff). Suggestion: Since the generation of the static files seems not so related to the refresh command (tell me if you see it differently 😉) I would think that adding a new command would be the most intuitive solution. Like What do you think? |
|
You got a good point. |
|
I've created a |
|
This looks better. Good job :) I will leave some more comments on the code, but nothing major. |
| * | ||
| * @param string $path | ||
| */ | ||
| public function generateMessagesFile($path) |
There was a problem hiding this comment.
Concerns generateMessagesFile as well as generateConfigFile:
If the only parameter is $path I would expect that this path is supposed to be the output file path. But here it is the path to the output file's parent directory.
Suggestion: In both methods let $path be the output file path. Either have two config options for the two files or leave it as it is and just do the $path = Config::get('js-localization.storage_path') . 'messages' in the fire() method.
src/Console/ExportCommand.php
Outdated
| if (!is_dir($path)) { | ||
| mkdir($path, '0777', true); | ||
| } | ||
| $path = $path . 'messages'; |
src/Console/ExportCommand.php
Outdated
| { | ||
| $config = ConfigCachingService::getConfigJson(); | ||
| if ($config === '{}') { | ||
| return; |
There was a problem hiding this comment.
In this case we should at least log a warning or something, so if the user expected a config file to be created they at least know why it's not there (because no configuration values are marked for export).
src/Console/ExportCommand.php
Outdated
| * @param string $messages | ||
| * @return string | ||
| */ | ||
| protected function ensureBackwardsCompatibility($messages) |
There was a problem hiding this comment.
This backwards-compatibility hack is pretty old. I suppose we can drop it for the new feature.
…rds compatibility.
|
Awesome, @Jarlskov! 👍👍 Just one more thing (sorry!) that came to my mind when reading the new code: Just an idea: How about passing the output directory as an argument to the export command? Might default to |
|
I'm not sure about what you mean by the last comment. The We could add a flag so you can add a path when you call the command, but I'm not sure why you would want to force it on each call, instead of allowing the user to set one path that's used every time. Or am I misunderstanding something? |
|
My line of thought was just: As a user when I want to run the export command the very first time I might expect it to ask me where to export. Otherwise I will wonder where it wrote the files to and how I can change it. So we might need to print a hint how the output directory can be changed. Might be easier to just let the user do But it's just a fresh thought. So you feel setting it once in the config might nevertheless be preferrable? |
|
Ok, I see the point. I'm just thinking that when you install the package, you have to open the config file, to specify what you want to export, so you already see the path in the config. I should probably add something about it to the README, though, so it's documented. |
|
Good point as well :) |
|
I've added usage instructions to the README.md :) |
|
Cool :) Will review very soon. Sorry that it takes a bit longer, but I am spending some days with my girlfriend. Got more time in a few days 😉 |
|
No worries, enjoy your time with her! :-) |
|
I just adapted the updated README a little bit: 60a85eb Tell me if you are fine with it :) |
|
Looks good, thanks for fixing my horrible sub-heading :-) |
Allow saving to disk
Possible fix for #29
I'm not sure if the storage + storage_path approach is good, of if I should rather go for splitting the storage 'file' option into a 'public' and 'storage' option so the user can choose whether files should be written to 'public' so it can be included straight in the markup, or 'storage' for use in JS compilation.