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

Improve (or avoid) the nojs experience #881

Closed
dcsjapan opened this issue Mar 20, 2016 · 20 comments
Closed

Improve (or avoid) the nojs experience #881

dcsjapan opened this issue Mar 20, 2016 · 20 comments

Comments

@dcsjapan
Copy link
Contributor

Inspired by this thread, following an update of discuss.flarum.org:

https://discuss.flarum.org/d/2303-locked-out-via-nojs

The message displayed on the basic HTML site could suggest that users try clearing their browser cache and reloading the page to see if that fixes the problem.

@tobyzerner
Copy link
Contributor

👍 we should also remove the redirection to nojs=1 so that they can hit refresh and it will attempt to boot the JS app again, rather than just displaying the nojs version

@JoshyPHP
Copy link
Contributor

Or maybe add a link to the JS-enabled page in the alert banner.

@franzliedke
Copy link
Contributor

Getting this nojs stuff right is tricky indeed. I think I suggested automatically redirecting back to the JS version with JavaScript. This takes care of those people that have JS disabled. But it can also cause infinite back-and-forth redirects if there is some JS error on the site.

How would this work without a redirect, @tobscure ?

I like the solution by @JoshyPHP best.

@tobyzerner
Copy link
Contributor

Maybe if we catch a JS error, we show an error message (using JavaScript) which says "please try reloading the page, or click here to view the HTML-only version"

@dsevillamartin
Copy link
Member

Couldn't we use something with the history APi to replace state to '/' but while they are in the noJs page?

@tobyzerner
Copy link
Contributor

@datitisev That's a very clever idea :)

@franzliedke
Copy link
Contributor

But it doesn't solve the problem for those people with JS disabled, once they enable JS and refresh the page, they will still see that error message. (Only then the history API will kick in.)

@tobyzerner
Copy link
Contributor

People with JS disabled won't be redirected to nojs=1 in the first place – they'll just see the <noscript> content, and when they enable JS and refresh, it will work

@franzliedke
Copy link
Contributor

Ah, I totally missed that part. ;)

@dsevillamartin
Copy link
Member

Glad I was able to help :)

Too bad I get my ideas in my sleep though.
I had to wait for morning and hope I would remember. ;)

How and when do you get redirected to the noJS page, then?
I can only think via server side (PHP), as the page doesn't even have time to load...

We should probably also try to integrate (don't know if it's the right word) an error system for the noJs page to indicate what went wrong and a "How-To Fix" message or something.


EDIT: Added +2 sections, "how & when" and "error & how to fix".

@ghost
Copy link

ghost commented Apr 5, 2016

It seems there is a fix for this, I found here:

https://laracasts.com/discuss/channels/laravel/how-to-force-browser-to-update-js-and-css-files-cache-after-deploy?page=1

It requires creating a versioning system for js files. So when an update is applied, the client will be forced to clear the cache from the older version.

@dcsjapan dcsjapan changed the title Add troubleshooting suggestions to the basic HTML page Improve (or avoid) the nojs experience Apr 5, 2016
@dcsjapan
Copy link
Contributor Author

dcsjapan commented Apr 5, 2016

👍 for ways to avoid the nojs experience altogether.

Troubleshooting suggestions and redirects would still be good to have, in case all else fails.

@franzliedke
Copy link
Contributor

@webeindustry We already add a version number to the JS files (or really, a hash). The problem in this case is probably that the HTML containing the references to old JS versions was cached by the browser - that should not happen, and it probably means we need to update our server configuration documentation.

@franzliedke franzliedke added this to the 0.1.0 milestone Apr 7, 2016
tobyzerner added a commit that referenced this issue May 26, 2016
- All custom JS variables are now preloaded into the `app.data` object, rather than directly on the `app` object. This means that admin settings are available in `app.data.settings` rather than `app.settings`, etc.
- Cleaner route handler generation
- Renamed ConfigureClientView to ConfigureWebApp, though the former still exists and is deprecated
- Partial fix for #881 (strips ?nojs=1 from URL if possible, so that refreshing will attempt to load JS version again)
@tobyzerner tobyzerner removed this from the 0.1.0 milestone Jul 22, 2017
@gwillem
Copy link
Contributor

gwillem commented Dec 2, 2017

@franzliedke are you sure it's a hash? See these files with same hashes, that are not the same:

https://greenmeteor.freeflarum.com/assets-nojs/admin-46889923.js
https://greenmeteor.freeflarum.com/assets-yesjs/admin-46889923.js

(php flarum cache:clear inbetween)

See also https://discuss.flarum.org/d/2303-locked-out-via-nojs/5

@sijad
Copy link
Contributor

sijad commented Dec 2, 2017

@gwillem I guess it's related to #837 (comment)

please try to remove rev-manifest.json

@tobyzerner
Copy link
Contributor

From my experience there is a bug in the asset compiler where the hash is only generated based on the modification timestamp of source files, but source strings are not taken into account.

So for example, if you alter custom LESS in the admin panel, the hash won't change and you'll need to clear your browser cache to force a reload.

@gwillem
Copy link
Contributor

gwillem commented Dec 15, 2017

There are multiple problems here.

Context: nearly every new Flarum user encounters nojs errors when clicking around the admin (as observed in the Free Flarum access logs). So it is quite a showstopper for Flarum adoption.

First problem: asset files are never re-hashed, because of this line. So they are only hashed when the revision for this file does not exist in rev-manifest.json. But RevisionCompiler.flush() does not clear the revision.

Second problem: the generated hash is based on a) file metadata b) strings (see this recent commit). But not on translations, which are defined here. So if the translations change (by adding an extension) the hash stays the same & the browser will not download the new JS.

Proposed solutions:

  1. RevisionCompiler.flush() calls putRevision() with an empty revision so the next getFile() will regenerate the hash and save a new file.
  2. RevisionCompiler uses a hash of the actual content instead of derived methods. This hash method is then used by getFile() for new filenames. This would not impact performance, because cache validation doesn't happen now anyway.

I would make a PR but my PHP skills are next to zero 🎉 (non-zero after debugging this ;))

PS. Observe that this bug is covered by the famous "there are only two hard things in computer science" categories ;)

@gwillem
Copy link
Contributor

gwillem commented Dec 15, 2017

Also, note that wiping rev-manifest.json in WebAppAssets.flush() as proposed by @tobscure (#1160 (comment)) will have bad side effects, as the flush is called multiple times (for forum and admin). That would delete the revision before the second flush could unlink its files.

@gwillem
Copy link
Contributor

gwillem commented Dec 15, 2017

Also, the generated hash for locale files is the same (deb59aba) for every Flarum install, as it is based on an empty string --> https://publicwww.com/websites/%22deb59aba%22/

@franzliedke
Copy link
Contributor

franzliedke commented Dec 15, 2017

the same (deb59aba) for every Flarum install

Ouch. 😁

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

No branches or pull requests

7 participants