Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch conf.yml from server #528

Merged
merged 12 commits into from
Mar 6, 2022
Merged

Fetch conf.yml from server #528

merged 12 commits into from
Mar 6, 2022

Conversation

azerioxal
Copy link
Contributor

@azerioxal azerioxal commented Mar 5, 2022

Ateroz ✨ Feature Medium Ateroz /master → Lissy93/dashy Commits: 12 | Files Changed: 10 | Additions: 16

Thank you for contributing to Dashy! So that your PR can be handled effectively, please populate the following fields (delete sections that are not applicable)

Category:
Feature

Overview
Fetch the config from a server endpoint. This allows for the conf.yml to be updated without a rebuild and sets a base for possible future config storage options such as a database. This should also allow for the releases to be smaller by not having to include the dev dependencies.

Thanks for this wonderful dashboard!

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors

@azerioxal azerioxal requested a review from Lissy93 as a code owner March 5, 2022 06:45
@viezly
Copy link

viezly bot commented Mar 5, 2022

Changes preview:

Legend:

👀 Review pull request on Viezly

@netlify
Copy link

netlify bot commented Mar 5, 2022

✔️ Deploy Preview for dashy-dev ready!

🔨 Explore the source changes: d0acd2f

🔍 Inspect the deploy log: https://app.netlify.com/sites/dashy-dev/deploys/6225404187e17d00088ac6b2

😎 Browse the preview: https://deploy-preview-528--dashy-dev.netlify.app/

@Lissy93
Copy link
Owner

Lissy93 commented Mar 5, 2022

Just reading through the changes now... This is awesome, nicely done :)
Been meaning to implement something similar for a while, thanks so much!

@Lissy93 Lissy93 added the ✨ New Feature [PR] Contains implementation of a new feature label Mar 5, 2022
Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just maybe it could work client-side, so that it still works when running as static web app. The config file can be fetched directly from axios.get('/conf.yml').

Could you also bump the version to 2.0.4 in the package.json, and add a quick summary to the .github/CHANGELOG.md

Other than that, all good, and solved a big problem, thanks a lot for submitting :)

src/store.js Outdated Show resolved Hide resolved
@azerioxal
Copy link
Contributor Author

azerioxal commented Mar 6, 2022

My bad, Netlify completely bypassed my thoughts when I was looking through the code and thinking about the changes. Doing it client side definitely works, but express serving both the dist and public folders causes some confusion.

I also just noticed that conf.yml is imported on build in InitServiceWorker.js. I've also noticed that they don't appear to be working correctly, and aren't enabled by default as mentioned in the docs. That's separate from this PR though.

Once I get all that fixed and make sure I haven't missed something like Netlify again, I'll push up the changes.
Thanks!

@azerioxal
Copy link
Contributor Author

azerioxal commented Mar 6, 2022

Okay, the changes you requested and a few others have been made.

  • I realized that I had missed InitServiceWorker.js and have corrected that
  • I edited the console messages to remove the mentions of rebuilding after changing the config

I also realized a problem caused by fetching directly from the client and the fact that the server is serving static files from both dist and public.
public is copied into dist when the Vue app is built. This results in two versions of conf.yml existing and the version in dist gets priority, causing any changes made in the UI and saved to disk to be ignored if say, the default conf.yml still exists. There a couple of ways that I could think of to fix it rather quickly.

  • Swapping around the static middlewares should change the priority, and load the correct file.
  • Only serving one folder.

I messed around with it for a bit and pushed a change to the Dockerfile that copies only the needed files for the app to work as a quick example. This lowers the size of the image by ~30mb, but doesn't do anything for an install from source.

My solution would be to split the server and frontend into two directories and only copy over the dist folder. This would also allow for two sets of dependencies which should make the docker image even smaller by being able to leave out all the dependencies that Vue needs to build.

I figured I'd push up what I've got so far and see which you prefer before rearranging anything and writing a build script.

Please let me know if I missed anything! And thanks again for this project, I've had a wonderful time working on it and I don't think I could have chosen better for my first contribution to open source!

@@ -14,11 +14,11 @@ import {
} from '@/utils/defaults';
import ErrorHandler from '@/utils/ErrorHandler';
import { applyItemId } from '@/utils/SectionHelpers';
import conf from '../../public/conf.yml';
import $store from '../store';
Copy link
Owner

@Lissy93 Lissy93 Mar 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use the @ alias for consistency, like: import $store from '@/store';

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Just one small thing, can you add the SET_REMOTE_CONFIG key to the list in StoreMutations.js. Works fine in production, but I get an undefined error when that's missing in dev. Once that's done, then ready for merge :)

@Lissy93 Lissy93 linked an issue Mar 6, 2022 that may be closed by this pull request
4 tasks
@azerioxal
Copy link
Contributor Author

Done, thanks!

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@Lissy93 Lissy93 merged commit 93911c2 into Lissy93:master Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ New Feature [PR] Contains implementation of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Dashy taking a while to load after server reboot
2 participants