Allow saving to disk

[Jarlskov]

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.

[andywer]

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 js-localization:export or something like that. It might trigger a refresh first and then create an up-to-date message and config file.

What do you think?

[Jarlskov]

You got a good point. I considered the command as a general refresh command, but reading the description again it actually says Refresh message cache. I'll create a new command. That would also allow removing the storage setting, and hopefully, prevent any confusion regarding the current blade tags.

[Jarlskov]

I've created a js-localization:export command for generating the static files.

[andywer]

This looks better. Good job :)

I will leave some more comments on the code, but nothing major.

[andywer]

Awesome, @Jarlskov! 👍👍

Just one more thing (sorry!) that came to my mind when reading the new code: The storage_path defaults to this package's directory. It's really hard to tell where the user will desire the newly created files to be written to.

Just an idea: How about passing the output directory as an argument to the export command? Might default to ./ (cwd).

[Jarlskov]

I'm not sure about what you mean by the last comment. The storage_path should be where the user desires the files, no?

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?

[andywer]

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 ./artisan js-localization:export ../frontend/externals/, a ./artisan js-localization:export without parameters just shows how to use the command. It's also easier to trigger a test export when debugging without overriding the existing output files, for instance.

But it's just a fresh thought. So you feel setting it once in the config might nevertheless be preferrable?

[Jarlskov]

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.

[andywer]

Good point as well :)

[Jarlskov]

I've added usage instructions to the README.md :)

[andywer]

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 😉

[Jarlskov]

No worries, enjoy your time with her! :-) Thanks for the help so far, it's been a great learning experience!

[andywer]

I just adapted the updated README a little bit: 60a85eb84629486545b76b1d4168faebb9e7cf87

Tell me if you are fine with it :) Ready to merge when you are 😉

[Jarlskov]

Looks good, thanks for fixing my horrible sub-heading :-) I'm fine with it, so if it seems useful, I have no problem putting my name on it.