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

Remove filehash setting from Trunk.toml #102

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

elliottslaughter
Copy link
Contributor

@elliottslaughter elliottslaughter commented Jun 27, 2023

By default, Trunk uses infinite caching. Without a file hash, this makes it quite easy for users to see stale copies of an application. The file hash (Trunk's default setting) ensures that users always load the exact hashed version that corresponds to what is currently deployed.

See also: https://github.com/thedodd/trunk/blob/master/Trunk.toml#L12-L13

Let me just add: I'm not sure why the setting was added in the first place, because the Trunk.toml contained no comments. But I've been wondering why Trunk has such a horrible caching policy for several months without realizing that eframe_template was setting an (arguably bad) non-default setting here.

By default, Trunk uses infinite caching. Without a file hash, this makes it quite easy for users to see stale copies of an application. The file hash (Trunk's default setting) ensures that users always load the exact hashed version that corresponds to what is currently deployed.
@emilk
Copy link
Owner

emilk commented Aug 8, 2023

Thanks!

@emilk emilk merged commit 1887323 into emilk:master Aug 8, 2023
6 checks passed
@elliottslaughter elliottslaughter deleted the patch-2 branch August 8, 2023 19:55
@coderedart
Copy link
Contributor

coderedart commented Aug 17, 2023

@elliottslaughter

I'm not sure why the setting was added in the first place, because the Trunk.toml

Its been a long time, but i enabled it for the sw.js script. My bad, i should have added a comment for that.

From README

assets/sw.js

Change the './eframe_template.js' to ./your_crate.js (in filesToCache array)
Change the './eframe_template_bg.wasm' to ./your_crate_bg.wasm (in filesToCache array)

I thought enabling file hashes would break the service worker caching.

I just checked.

  • with the current patch where we enable file hashing, the app fails to load if i go offline.
  • with the old config where we disable file hashing, the app will work fine even when i go offline. Ofcourse, we haven't added assets like icons to the caching list, so they will be missing when i am offline, but wasm/js/index.html work fine.

I personally don't really use the offline service worker caching feature, so i am fine. But sw.js needs to be edited to consider the hash in name. And also make sure that it only keeps the most recent version, so that older files will be removed.

@elliottslaughter
Copy link
Contributor Author

Thanks for the explanation. If I understand correctly (unless we figure out a fix with Trunk rewriting sw.js to mention the files by hash), we're being forced into the following tradeoff:

  1. Either we get working service worker support (i.e., the ability to load everything while offline), or
  2. We get support for correctly reloading the deployed version without clearing all cache and service workers

For my personal use case, (2) is the much bigger deal. Users keep getting old versions of the app, and explaining how to properly update is tricky (I had users who couldn't follow the instructions and were basically permanently stuck on a broken app).

But up to you guys what you want to prioritize, or whether to pursue a real fix in Trunk or not.

@coderedart
Copy link
Contributor

coderedart commented Aug 17, 2023

Yeah. A lot of people have issue with the caching problem. that is why we have a big warning in README

Open http://127.0.0.1:8080/index.html#dev in a browser. See the warning below.
assets/sw.js script will try to cache our app, and loads the cached version when it cannot connect to server allowing your app to work offline (like PWA). appending #dev to index.html will skip this caching, allowing us to load the latest builds during development.

and another warning in the html file

    <!--Register Service Worker. this will cache the wasm / js scripts for offline use (for PWA functionality). -->
    <!-- Force refresh (Ctrl + F5) to load the latest files instead of cached files  -->
    <script>
        // We disable caching during development so that we always view the latest version.
        if ('serviceWorker' in navigator && window.location.hash !== "#dev") {
            window.addEventListener('load', function () {
                navigator.serviceWorker.register('sw.js');
            });
        }
    </script>

You just need to remove the above html from index.html and you won't be caching anything.

The description of the PR did confuse me. The caching behavior doesn't come from Trunk AFAIK, but sw.js script.

@emilk Well, its not the first time this has come up. There are three open issues all related to this Progressive Web App behavior.

#99
#84
#62

Maybe we should comment out the service worker html, so that things will work fine for most people by default. And the people who care about it (PWA) can explicitly choose to uncomment that part.

@c-git c-git mentioned this pull request Oct 2, 2023
@9SMTM6
Copy link

9SMTM6 commented Jun 23, 2024

With the current setup we get errors in the application, because the Serviceworker tries to load stuff that doesn't exist (the files without filehash that is). This seems to only trigger with actually deployed apps (service workers only start in certain circumstances) so this may be why it was missed.

Also, because of the warnings etc, one kind of assumes that the app is offline capable when it actually isn't with the default config.

It would be kindof possible to hack in 'proper' support for caching by adding hooks into trunk, but its not going to be easy, its going to be fragile, and probably beyond the scope of this starter template. The real proper way would be to add it to trunk, but there doesn't seem to be any effort on this.

I thus would suggest removing the traces. As nice as it is to have a kinda working setup, at this point the drawbacks outweight the advantages for most, and to have it working properly one has to put in a lot of work and complicate the application by quite a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants