search

arnoson / kirby-vite

Use Kirby CMS together with Vite
MIT License
81 stars 7 forks source link

vite-kirby-plugin enhancements #29

Closed bogdancondorachi closed 1 year ago

bogdancondorachi commented 1 year ago

Hi @arnoson, I'm back with some improvement ideas :)

Issues

  1. The current vite.config file generates with an '\' path separator which I found that on some of my projects, it has to be '/' in order to find the right directory path
  2. Let's say you modify the vite.config file to your needs but every time you run vite the config file overrides itself

Fixes

  1. By using sep from the path module, the script will automatically use the appropriate separator for the operating system. This ensures that the outDir path will be formatted correctly for the current OS
  2. I added a try-catch block around the access function to check the vite.config file already exists. If the file exists, it will skip the automatic override. This way, if you manually edit the file, the script won't override the changes.

Let me know your thoughts on these changes :)

arnoson commented 1 year ago

Hi @bogdancondorachi, thanks for the PR and fixing directory seperators, I have to admit I never tested them on windows... The PR looks good but I'm not sure about issue 2. For me the vite.config.js is the only source of truth and all config should happen there. The generated vite.config.php just shares this config with PHP, so it should be always overwritten to be in sync. Maybe a header comment could be added to vite.config.php to make this more clear, like // this is an auto-generated file, please make all configuration in your vite.config.js. Is there any scenario where would need diverging config files?

bogdancondorachi commented 1 year ago

Yeah, tbh I didn't have the issue on Windows with this path as it was the correct one for the OS, just on a client Linux server where it had to be '/' on Unix in order to find the correct path to the outDir.

Regarding the rewriting of the vite.config, you're right, I've also thought of it after the PR that it can cause further inconsistency issues (only use case scenario would be path differences between local/production but with the correct sep fix it kind of fixes the issue if you also do the build on production). Certainly, it should contain a commit to specify the purpose of the file. Also, I think it does not make sense to make unnecessary writes if the file exists and the values are the same.

I've added another commit where we check if the file exists already & has the same contents before attempting to write it, and the specific purpose commit.

Let me know your thoughts and if you need a new cleaner PR :)

arnoson commented 1 year ago

Thanks :) Made some minor code changes and had to run the release script again, because the git tag somehow got lost in the PR

bogdancondorachi commented 1 year ago

LGTM, thank you for pushing this :)