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

nodejs.org: falling back to english when page hasn't been translated #316

Closed
wants to merge 3 commits into from

Conversation

phillipj
Copy link
Member

Refs nodejs/nodejs.org#490

Currently translation working groups will have to translate all pages to their respective
language, if not most pages would end in displaying a (english) 404 page. That's a massive
requirement on translation groups, and keeping up with future changes are probably near impossible.

Lowering the barrier and letting translation groups start on the frontpage and work their way
down a couple of page depths, should be alot simpler and more rewarding.

NB! This only affects local development as nodejs.org runs nginx serving strictly static content.
Getting the same behaviour in production requires changes to the nginx config.

Example of the current 404 issue; navigate any of the headers links in the current Korean translation: https://nodejs.org/ko/. I haven't done my dues when it comes to translating (yet), this is one of the main reasons I've decided not to.

@jbergstroem
Copy link
Member

This will make all 404's redirect to /en/ for the nodejs.org scope (besides dist) - right? Not sure that is the desired effect.

@phillipj
Copy link
Member Author

phillipj commented Feb 2, 2016

Good question. Got to admit I didn't think about URLs other than the actual
website. I'll dig more into that as soon as I'm back from one week vacation.

On Tuesday, 2 February 2016, Johan Bergström notifications@github.com
wrote:

This will make all 404's redirect to /en/ for the nodejs.org scope
(besides dist) - right? Not sure that is the desired effect.


Reply to this email directly or view it on GitHub
#316 (comment).

@phillipj
Copy link
Member Author

This will make all 404's redirect to /en/ for the nodejs.org scope (besides dist) - right?

I assume you would rather see it check the /en/ equivalent exist before doing a redirect, correct? Haven't been able to get that working... Got a trick up your sleeve @jbergstroem?

@phillipj
Copy link
Member Author

phillipj commented Mar 2, 2016

To elaborate on what I've tried to accomplish without luck so far:

location @english_fallback {
    set $english_filename '';

    if ($request_filename !~* /en/) {
        set $english_filename ...; # need string replace??
    }

    if (-e $english_filename) {
        rewrite ^(/\w+/)(.*)$ http://nodejs.org/en/$2;
    }

    return 404;
}

The biggest challenge here is doing a string replace on the current $request_filename to get the english equivalent need to do the existence check. AFAIK the kind of manual string replace functionality needed, would require nginx_substitutions_filter to be installed.

Anyway the use if is strongly discouraged by the nginx team and community...

@jbergstroem
Copy link
Member

How about going at it the other way? redirecting all other 'known' languages that doesn't exist to en?

@phillipj
Copy link
Member Author

phillipj commented Mar 7, 2016

@jbergstroem something like this?

location @english_fallback {
    rewrite ^/(it|ko)/(.*)$ http://nodejs.org/en/$2;
    return 404;
}

That surely removes the complexity and no if's in sight 👍

@phillipj phillipj force-pushed the fallback-to-english branch 2 times, most recently from 0a34b07 to 224dcdb Compare March 9, 2016 21:59
@phillipj
Copy link
Member Author

phillipj commented Mar 9, 2016

@jbergstroem just updated the PR with the simplified version posted above ^^ with explicit known languages as you suggested. Also added the english fallback to the https server directive.

@phillipj
Copy link
Member Author

@jbergstroem any thoughts on this after it got updated?

@Fishrock123
Copy link
Contributor

Didn't we have something like this for iojs.org?

Looks like it does work on iojs.org: https://iojs.org/fa/es6.html

Ideally, however, it would not re-write to /en/ so that any future links can still go to the native language? I think? I'm not sure.

@Fishrock123
Copy link
Contributor

Hmm, looks like we just made redirect static files for each page: https://github.com/nodejs/iojs.org/blob/master/source/static/es6.html

@Fishrock123
Copy link
Contributor

As a note, 404's don't even work correctly at the current time: https://nodejs.org/ko/404/ - yet we clearly have one for korean: https://github.com/nodejs/nodejs.org/blob/master/locale/ko/404.md

@@ -45,6 +45,8 @@ server {
}

location / {
try_files $uri $uri/ @english_fallback;

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@Fishrock123
Copy link
Contributor

Ok I spent some time trying to get the localized 404 pages working today.

What I've come up with:

location @english_fallback {
    if ($uri ~* ^/(it|ko)/) {
        set $lang $1;
    }
    rewrite ^/(it|ko)/(.*)$ /en/$2; # I've used a relative url here for testing, not sure if it should be absolute or not
    # No `return 404;` here, I think that makes our page actually return that status code ontop of serving..
}

Where we currently declare error_page 404 /en/404.html;:

error_page @404;
# use localized 404 pages if possible
location @404 {
    try_files /$lang/404.html /en/404.html;
}

@Fishrock123
Copy link
Contributor

@phillipj Hopefully you have time to look at this, I'd definitely like to get it live!

@jbergstroem
Copy link
Member

I will follow up here shortly too.

@phillipj phillipj force-pushed the fallback-to-english branch 2 times, most recently from ebf0e0b to e8d291e Compare April 25, 2016 14:42
@phillipj
Copy link
Member Author

@Fishrock123 awesome, thanks for pitching in here! Just pushed an update which contains your localized 404 hackery 👍

if ($uri ~* ^/(it|ko)/) {
set $lang $1;
}
rewrite ^/(it|ko)/(.*)$ /en/$2;

This comment was marked as off-topic.

@Fishrock123
Copy link
Contributor

@nodejs/build could some people look at this please? This is really important for website translation and I'd like to get it live ASAP.

@phillipj phillipj force-pushed the fallback-to-english branch from e8d291e to ef8b01b Compare May 2, 2016 18:17
@phillipj
Copy link
Member Author

phillipj commented May 2, 2016

Rebased to fix merge conflict.

Also decided to not go for relative rewrites to do things as before, and not raising more questions than necessary.

@Starefossen
Copy link
Member

Looks good to me 👍

@jbergstroem
Copy link
Member

Sorry for the delay -- I just need to do some performance testing (if's are evil yada yada)

@Fishrock123
Copy link
Contributor

ifs might be evil but I don't know another way to do it. :(

@Fishrock123
Copy link
Contributor

@jbergstroem Can we merge this and if it really turns out to be a problem, undo it and have a single, multi-language 404 page? This is badly blocking translation groups as I understand... I know you're busy. :(

(I don't really know how to benchmark this... if you just want me to run ab or wrk against it I can do that though!)

@jbergstroem
Copy link
Member

@phillipj I'd suggest having a ubuntu14 machine available and override hosts iojs-www and node-www when you run the playbook then. You'll have to add those hosts to your ssh config.

@rvagg
Copy link
Member

rvagg commented May 31, 2016

I've been messing with setting up a test machine for you to play with this on, knowing that our ansible scripts are not entirely solid for the web server this was a good chance to try and address that.

@phillipj I've put your ssh keys from GitHub into root@107.170.115.122 and it's set up almost identically to the nodejs.org server and you can do what you like with it. Stick 107.170.115.122 nodejs.org into your /etc/hosts to make it work like the real thing for you. /home/www/nodejs/ is where the web files are. Run /home/nodejs/build-site.sh nodejs if you want to rebuild the web site from the nodejs.org/node repo. /etc/nginx/sites-available/nodejs.org is where the nginx config for the site is that you can play with. This machine is not entirely devoid of production secrets so don't go liberally sharing access to it, in fact if you want to give someone else access you might want to ask here about that.

For the record: the playbook in this repo for www should be in a better state now, it can be run with ansible-playbook -i <path to inventory file with a [node-www] block and a host listed under it> ansible-playbook.yaml but the tricky bit is that you'll need some production secrets available to do a proper deploy, faking them out as things fail is one way to go, some info about what's needed can be gleaned from resources/secrets/makesecrets.sh too.

@rvagg
Copy link
Member

rvagg commented May 31, 2016

@Fishrock123 I put your keys in there too if you're interested in shaving this yak

@Fishrock123
Copy link
Contributor

Noted, looking at it right now.

@Fishrock123
Copy link
Contributor

Hmm, Indeed it does not appear to be working.

Aside: why do we have 2 server configs for both blog.nodejs.org and nodejs.org? It's quite confusing.

@rvagg
Copy link
Member

rvagg commented May 31, 2016

@Fishrock123 legacy, you'll note that all of the blog redirects are contained within the blog.nodejs.org part of the config, which leave the main nodejs.org config with much fewer redirects in its flow.

I think that's what you're referring to. Or perhaps you're seeing :80: vs :443. Again, legacy, the ideal situation is to have a minimal :80 but we've yet to fully make that cutover because we've been allowing http access to /dist/ for legacy reasons. We'll get there soon, I think in the next few months in fact.

@rvagg
Copy link
Member

rvagg commented May 31, 2016

Also, you're welcome to propose improvements to our nginx config, for both readability and actual efficiency. Just be aware of the legacy constraints, things need to continue to work as they do now. Mostly I'm pretty afraid to mess with nginx config too much because it's a bit like playing wac-a-mole.

https://github.com/nodejs/build/blob/master/setup/www/resources/config/nodejs.org

@phillipj
Copy link
Member Author

phillipj commented Jun 3, 2016

@rvagg thanks for adding my keys!

After some trial-n-error, it seems like using relative URLs in the fallback rewrites made a positive impact. I just pushed a commit. Those changes has been made on 107.170.115.122 if some of you want to do a quick check, by adding it into /etc/hosts as mentioned in #316 (comment).

@phillipj
Copy link
Member Author

phillipj commented Jun 4, 2016

Added zh-cn in addition to it|ko cause of recently added translations (e.g. nodejs/nodejs.org#771 & nodejs/nodejs.org#776)

location / {
try_files $uri $uri/ @english_fallback;

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@rvagg
Copy link
Member

rvagg commented Jun 8, 2016

sgtm but I'm way too gun shy with this nginx config to be comfortable signing off on it. Can others who are comfortable hacking nginx config have a look and try it out on the test host too? @jbergstroem?

@jbergstroem
Copy link
Member

I will follow up shortly to confirm that it works as intended. I wasn't a big fan of how variable scopes were used; but it was indeed nifty.

@Fishrock123
Copy link
Contributor

At some point we're going to have to just try it.

@Fishrock123
Copy link
Contributor

... ping again ...

@rvagg
Copy link
Member

rvagg commented Jun 27, 2016

I'm OK with giving this a try, but only if @jbergstroem is +1 too. We should probably do it during a low traffic period, the weekend I guess, as long as we have people around to revert quickly if need be.

@jbergstroem
Copy link
Member

Not sure which timezone digitalocean runs by, but least amount of traffic occurs in ~13h from now. I can attempt to give it a test around then.

@jbergstroem
Copy link
Member

This has been running for a bit on nodejs.org now and seems to be working as intended.

@Fishrock123
Copy link
Contributor

Has been running for about 3 days now? I think this can be merged

@jbergstroem
Copy link
Member

I'll merge this!

@phillipj
Copy link
Member Author

@jbergstroem any progress on merging this? nodejs.org just got a new locale (es / spanish), so it would be nice to get this merged before opening a new PR adding spanish as well.

@jbergstroem
Copy link
Member

@phillipj I'll add es/spanish.

jbergstroem pushed a commit that referenced this pull request Jul 28, 2016
Fall back to english if a page hasn't been translated.

Refs: nodejs/nodejs.org#490
PR-URL: #316
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jbergstroem
Copy link
Member

Merged (with the addition of es) in 9488a61.

@phillipj
Copy link
Member Author

Awesome 👍

On Thursday, 28 July 2016, Johan Bergström notifications@github.com wrote:

Merged (with the addition of es) in 9488a61
9488a61
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#316 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABLLE96B7JlTVHmgHr8BZD05RrK_q6jQks5qaLJYgaJpZM4HPlRm
.

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.

5 participants