Split i18n engine to specific parts by tech and by env#20513
Split i18n engine to specific parts by tech and by env#20513yankouskia merged 6 commits intoelastic:masterfrom
Conversation
yankouskia
added
Team:Core
💚 Build Succeeded |
azasypkin
left a comment
azasypkin
left a comment
There was a problem hiding this comment.
I like the idea in general. I left a few questions and one concern regarding mixing of kbn/i18n reorg changes with intl polyfill, I'd rather split this into two separate PRs.
There was a problem hiding this comment.
question: is there any reason why we need main (same question for react)?
There was a problem hiding this comment.
Yes. React and Angular is built in order to be used in Node as well as in browsers (even it will be changed, believe that devs will add 2 different sections as well). And we use Node environment in tests, and potentially can use ssr where Node env as well. That's why I split it to 2 parts. Another point - it's babel build. It is just fortuity, that client-preset is working on server, so we should be consistent with it.
packages/kbn-i18n/package.json
Outdated
There was a problem hiding this comment.
question: why do we need --copy-files?
There was a problem hiding this comment.
We need to copy package.json for correct resolving modules
packages/kbn-i18n/package.json
Outdated
There was a problem hiding this comment.
question: I still see target/node/angular|react, would it make sense to use babel src/core in build:node or something like that to avoid unnecessary files in target/node?
There was a problem hiding this comment.
issue: looks like intl polyfill is out of scope for this particular PR, can we add it in a separate PR and not mix two unrelated changes?
There was a problem hiding this comment.
Yes, actually you are absolutely right, but I added because of 2 reasons:
- It is really small change and easy to review, but if I would add it in another PR, it would be blocked.
- In this PR I added support for different envs. And would like to deliver fully working code without
Intlissues
There was a problem hiding this comment.
It is really small change and easy to review
Hmm, can't agree here, it's a new dependency and we should know and discuss all the consequences it may have (e.g. build size). And it's for node-only, does it block anything specific right now in Index Management localization effort?
but if I would add it in another PR, it would be blocked.
That's totally fine, let's solve issues when they arrive.
There was a problem hiding this comment.
nit: I'd still keep default index.js for node and browser.js for browser similar to what is used by npm itself: main for node and browser for browser.
There was a problem hiding this comment.
you are right, will fix it, thank you!
There was a problem hiding this comment.
question: btw, why we do we need core/i18n subfolder, can't we use just core?
There was a problem hiding this comment.
I did it because of next reason: this functionality is used in Angular and React wrappers, and here we should also split into browser and node implementation as React for instance is the same for both. Also, I split real core from loader, where we do not have any environment specific code.
💔 Build Failed |
💚 Build Succeeded |
azasypkin
left a comment
azasypkin
left a comment
There was a problem hiding this comment.
LGTM, the only thing that I'd do differently is to move core/index.js and core/lodader.js into src/ root and put everything from core/i18n into core directly to reduce seemingly unnecessary nested i18n folder. But please tell me if I'm missing something.
💚 Build Succeeded |
yankouskia
force-pushed
the
feature/partial-i18n
branch
from
44132a9 to
4c236db
Compare
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
* split to packages in accordance to specific library * split to packages depending on tech and environment * make env modules names consistent * remove intl polyfilling * move laoder to root, i18n folder to core
* split to packages in accordance to specific library * split to packages depending on tech and environment * make env modules names consistent * remove intl polyfilling * move laoder to root, i18n folder to core
|
6.x/6.5: 7026148 |
5 participants
Split i18n engine to specific parts by technology (
core,React,Angular) and by environment (BrowserandNode)#20466