[uiExports] migrate uiApp "uses" to explicit imports in apps #17828
Conversation
spalger
force-pushed
the
implement/uiExports-modules
branch
2 times, most recently
from
343f067
to
7261884
Compare
💚 Build Succeeded |
|
@spalger could you please rebase PR on the latest master (it doesn't block review though, already on it)? |
| @@ -93,7 +80,7 @@ export class UiApp { | |||
| } | |||
|
|
|||
| getModules() { | |||
There was a problem hiding this comment.
question: are we going to get rid of this method any time soon? If no, maybe it'd make sense to rename getModules to getMain/getMainModule or something like this and return just this._main.
There was a problem hiding this comment.
Yeah, they'll be gone soon which I why I kept the clean up minimal.
| @@ -67,5 +67,6 @@ afterEach(function () { | |||
|
|
|||
| // Kick off mocha, called at the end of test entry files | |||
| export function bootstrap() { | |||
| require('uiExports/hacks'); | |||
There was a problem hiding this comment.
nit: maybe you can add comment here, why we need this new require like you did in chrome.js (unless it's obvious for everyone else except me :))?
src/optimize/base_optimizer.js
Outdated
| // what require() calls it will execute within the bundle | ||
| JSON.stringify({ | ||
| type, | ||
| modules: [].concat(extensions[type] || []) |
There was a problem hiding this comment.
question: why not just extensions[type] || []? Do we support non-arrays in extensions[type]?
There was a problem hiding this comment.
You're right, the values in appExtensions will always be arrays.
| const { type, modules } = JSON.parse(module.id.slice(module.id.indexOf('?') + 1)); | ||
|
|
||
| const requires = modules | ||
| .sort((a, b) => a.localeCompare(b)) |
There was a problem hiding this comment.
question: just curious, what is value of localeCompare in this particular use case? Is there cases when default comparison won't work?
There was a problem hiding this comment.
Just a habit when comparing strings.
| } | ||
|
|
||
| export default function () { | ||
| if (module.id.indexOf('?') === -1) { |
There was a problem hiding this comment.
nit: includes('?') or maybe indexOf but put result into variable and reuse below?
| .sort((a, b) => a.localeCompare(b)) | ||
| .map(m => { | ||
| const req = normalizePath(m); | ||
| return `{ req: '${req}', module: require('${req}') },`; |
There was a problem hiding this comment.
nit: would be great if you can add a comment to clarify why we use this format exactly ({ req, module}) and where it's consumed.
There was a problem hiding this comment.
That format was beneficial for the approach I alluded to in the description, where uiExport modules were being pulled in by the registries rather than being pulled in to decorate the registries... No need for it now.
spalger
force-pushed
the
implement/uiExports-modules
branch
from
f9f1543
to
ce4e7b0
Compare
|
@azasypkin @epixa updated to address review feedback and include the x-pack pr (sorry for the rebase) |
💚 Build Succeeded |
|
Still LGTM :) |
💚 Build Succeeded |
* [uiExports] migrate uiApp "uses" to explicit imports in apps * [uiApp] update tests for getModules() method * [optimize/uiExports] improve naming and comments * [uiExports] sort imports so they load in the same order as before * [testHarness] load hacks when testing in the browser * [x-pack/uiExports] use new uiExports modules * [testHarness] describe why we import uiExports/hacks * [optimize] remove needless [].concat() * [optimize/createUiExportsModule] string.includes > string.indexOf * [uiExports/createUiExportsModule] remove needless capture of module exports (cherry picked from commit e1a2fcb)
|
6.4/6.x: a6c0c62 |
Fixes #17481
Rather that defining the uiExports that an app "uses" in its uiExport spec, apps will now need to import the uiExports in their main/entry file.
This allows us to move closer to the notion of an app only existing on the client side, as the optimizer no longer needs to know what uiExports to include in each bundle.
Before you ask 😁: I had a version of this PR that moved the import/require calls into the registries so that apps wouldn't need to know what uiExports they should load to populate the registries they want to use. While I made promising progress towards this goal, the changes necessary were pretty large and I started to realize it wasn't worth the risk for a change that will be made obsolete by the work I'm doing on the new platform.
cc @timroes @nreese @w33ble @weltenwort