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

Build default language with locale (adds preserveLocale flag) #1940

Merged
merged 6 commits into from
Jan 20, 2019

Conversation

flakolefluk
Copy link
Contributor

Currently when navigating to a page that does not have a locale, it will default the english one, this breaks the navigation (it doesn't really break it, but everything including links now changes locale to english)
2018-12-15 01 11 30
This PR adds a preserveLocale flag that will run a second build of the files not included in the locale that exist in the "en" locale but using the current locale site.json.
Build time is considerably longer (en locale has almost 700 files, compared to others with 20 or less)
The result:
2018-12-15 01 16 14

Open for discussion.

Copy link
Member

@ZYSzys ZYSzys left a comment

Choose a reason for hiding this comment

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

It's awesome for me !👍

But is there anything we can do to make a bit more optimization ? That's even better :)

@flakolefluk
Copy link
Contributor Author

As far as i know, there's no way to avoid building all the pages available in english (ignoring the ones already built for the current language) for every locale. I'm thinking of adding specific language builds, but i guess that is not part of this PR. Any advice?

@flakolefluk
Copy link
Contributor Author

Looks like it solves #825

@ZYSzys
Copy link
Member

ZYSzys commented Dec 19, 2018

/cc @fhemberger Can you please have a review on this, although I'm almost +1 for this(feature maybe).

@@ -5,10 +5,11 @@
"homepage": "https://nodejs.org",
"scripts": {
"build": "node build.js",
"build:deploy":"node build.js --preserveLocale",
"serve": "node server.js",
Copy link
Member

@ZYSzys ZYSzys Dec 19, 2018

Choose a reason for hiding this comment

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

Since we also have preserveLocale in server.js, is it more reasonable to serve as node server.js --preserveLocale ? Or add one more script like build:deploy but for serve ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think while developing is less needed to build all the files for every language. I think it's more relevant when deploying. But a second script in package.json might help.

Current options to serve with preserveLocale are:

npm start -- -- --preserveLocale
npm run serve -- --preserveLocale
node server.js --preserveLocale

I could add a script to run:

npm run serve:preserveLocale

Copy link
Contributor

@fhemberger fhemberger left a comment

Choose a reason for hiding this comment

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

If we decide to land this change, this would be a good opportunity to go over the process in general.

E.g. instead of building all languages for local testing/serving, setting DEFAULT_LOCALE=en by default and allow collaborators to overwrite that with an environment variable (e.g. DEFAULT_LOCALE=zh-cn npm run serve, when you're only working on the Chinese translation).

Only for deployment via CI all languages must be built.

This would save a lot of generation time for local development.

build.js Outdated Show resolved Hide resolved
Copy link
Contributor

@fhemberger fhemberger left a comment

Choose a reason for hiding this comment

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

First of all: Thank you for your work! 👍

Only two little nits remaining:
a) See comment below
b) Could you add a short description of the build options (as in your comment above) to the README file?

FYI, I measured the time difference for a full build with and without --preserveLocale:

npm run build  12.83s user 1.62s system 118% cpu 12.215 total

rm -r build

npm run build -- --preserveLocale  113.75s user 9.43s system 125% cpu 1:38.04 total

As you can see, the latter took eight times as long. This probably isn't relevant during development, but this will have an impact on the build time during an actual release.

Therefore I'd love other members of @nodejs/website have a look as well.

server.js Outdated Show resolved Hide resolved
@flakolefluk
Copy link
Contributor Author

@fhemberger I added the requested changes to the readme section and when serving a different locale than english

@fhemberger
Copy link
Contributor

@flakolefluk Thanks, LGTM now.
@ZYSzys If you are okay with it, we can merge it.

build.js Outdated Show resolved Hide resolved
@ZYSzys ZYSzys merged commit 18966f6 into nodejs:master Jan 20, 2019
@ZYSzys
Copy link
Member

ZYSzys commented Jan 20, 2019

Thank you!👍

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.

3 participants