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

FES + i18n: Load translations from a CDN #4701

Merged
merged 5 commits into from
Jul 29, 2020

Conversation

akshay-99
Copy link
Member

@akshay-99 akshay-99 commented Jul 20, 2020

Resolves #4694

Changes:

These changes would take effect from the next release. Before then, the configured path ( https://cdn.jsdelivr.net/npm/p5/translations/ ) would give us a 404. This wouldn't be a problem for contributors because of the next point below.

  • Use local translation files in dev mode. Whenever someone is adding/updating something, they should be able to test it out. Fetching from the CDN will not be helpful as these changes wouldn't have been published yet. I configured grunt and browserify to pack all the translation files in the library when running browserify:dev ( i.e with npm run dev but not with npm run build )

Also, since we have updated our default branch name to main, we may have to change something with np to let it know this. Otherwise it complains that we aren't on the correct branch and would stop the release. For testing out this PR, I just manually passed in --any-branch. But there must be some other way to let np know the name of the default branch.

PR Checklist

@limzykenneth
Copy link
Member

limzykenneth commented Jul 21, 2020

It is possible to publish the files to cdnjs as well, probably just need to make a PR to edit this file.

A fix for np might be coming soon: sindresorhus/np#558. In the meantime to release from main, --any-branch should be used for now. I'll put together a PR for this for the time being.

@akshay-99
Copy link
Member Author

It is possible to publish the files to cdnjs as well, probably just need to make a PR to edit this file.

Yes. I will look into it.

@lmccart
Copy link
Member

lmccart commented Jul 23, 2020

This looks great. Could we add some link in the contributor_docs/ to the files that are necessary to update when adding another language? (And any other relevant links) I think it would go in the internationalization.md file.

@akshay-99
Copy link
Member Author

Hi. Yes. I will add that too.

@akshay-99 akshay-99 force-pushed the i18n-host-translations-online branch from ec0fcb5 to d3813a3 Compare July 26, 2020 08:11
@stalgiag stalgiag self-requested a review July 27, 2020 21:45
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Great! Two small simple questions inline. I am working with the assumption that we will have some time to look at this in more depth later to make sure that it is easy enough for others to maintain and build upon.

tasks/build/browserify.js Outdated Show resolved Hide resolved
tasks/build/browserify.js Outdated Show resolved Hide resolved
@akshay-99
Copy link
Member Author

akshay-99 commented Jul 29, 2020

I am working with the assumption that we will have some time to look at this in more depth later to make sure that it is easy enough for others to maintain and build upon.

One part that might be a bit difficult to maintain is this. There are already several i18next plugins available that do something similar like i18next-http-backend, i18next-fetch-backend. But there were two things which I didn't find a way to implement using these: quickly skip a language if it doesn't exist in the list of supported languages ( don't send a request ), and timeout a request if it takes too much time.

Since we are stalling the initialization waiting for the translations to finish downloading, it may be important that this step doesn't take too much time as well ensure that it doesn't spend time trying to find files for languages which we know aren't supported yet. This may be important when there is a chain of unsupported languages. For example, if someone's browser language is fr-CA ( Canadian French ), i18next will try to find fr-CA and since that isn't supported yet, try looking up fr. Since that too isn't supported, it finally comes to English. But it just spent time on two requests, which is easily prevented by just checking against a list of supported languages.

@akshay-99
Copy link
Member Author

I think adding a language itself should be easy for contributors since ( apart from the actual translation itself ) it only involves adding two extra lines in the code, here and here.

@akshay-99 akshay-99 force-pushed the i18n-host-translations-online branch from 7234cdf to 362adf8 Compare July 29, 2020 01:39
@stalgiag stalgiag merged commit 4fc242d into processing:main Jul 29, 2020
@stalgiag
Copy link
Contributor

Since we are stalling the initialization waiting for the translations to finish downloading, it may be important that this step doesn't take too much time as well ensure that it doesn't spend time trying to find files for languages which we know aren't supported yet. This may be important when there is a chain of unsupported languages. For example, if someone's browser language is fr-CA ( Canadian French ), i18next will try to find fr-CA and since that isn't supported yet, try looking up fr. Since that too isn't supported, it finally comes to English. But it just spent time on two requests, which is easily prevented by just checking against a list of supported languages.

Thanks for explaining this in detail. I think that quickly skipping unsupported languages should be a priority for future work.

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.

FES + i18n: Loading translation files from CDN
4 participants