-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
refactor(v2): i18n cleanups / refactors #4405
Conversation
[V1] Deploy preview failure Built without sensitive environment variables with commit 6bfdc1a https://app.netlify.com/sites/docusaurus-1/deploys/604ba7164f592500086a4d43 |
Deploy preview for docusaurus-2 ready! Built without sensitive environment variables with commit 6bfdc1a |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4405--docusaurus-2.netlify.app/classic/ |
- remove intl-locales-supported & intl since they're deprecated and only needed for IE11 - add new polyfills for node 12 - clean up babel intl extractor - reset jest test timezone to UTC so it passes even for East Coast contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up my AST processor :)
Not sure we want to remove intl-locales-supported, we don't use it in the browser but we actually use it for Node 12 lack of full ICU support.
We thought with @lex111 it would be simpler to load a polyfill in Node than asking our users to add full-icu support to Node 12. This is just a temporary solution until we require Node 14 as minimum version, including full ICU support.
Does it make sense to you?
Gotcha. What portion of docusaurus runs in Node? I thought it's client side only? |
Thanks, that looks good enough to merge. I'll consider the doc warning to use Node 13+ or full icu is good enough, but we'll see if users get confused and add a more strict runtime error if needed |
cc @slorber