-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remaining default to named conversions in ui/public #11323
Remaining default to named conversions in ui/public #11323
Conversation
jenkins, test this |
And turned on the eslint rule for that folder.
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.
Two questions, but otherwise it's a clean PR.
@@ -29,7 +29,8 @@ require('./vis'); | |||
SavedObjectRegistryProvider.register(require('plugins/timelion/services/saved_sheet_register')); | |||
|
|||
// TODO: Expose an api for dismissing notifications | |||
const unsafeNotifications = require('ui/notify').default._notifs; | |||
import { notify } from 'ui/notify'; |
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.
Are we okay with middle-of-the-file import
s?
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.
moved to the top!
require('ui/utils/lodash-mixins/function')(_); | ||
require('ui/utils/lodash-mixins/oop')(_); | ||
const _ = require('node_modules/lodash/index.js').runInContext(); | ||
const { lodashStringMixin } = require('ui/utils/lodash-mixins/string'); |
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.
Should these become import
s?
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.
I believe this file is excluded from the babel transformation so if I make them imports, I get errors:
ERROR in ./webpackShims/lodash.js
Module parse failed: /Users/staceygammon/Elastic/kibana/webpackShims/lodash.js Line 10: Unexpected token
You may need an appropriate loader to handle this file type.
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.
ok, similar comments as @pickypg
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.
🥇
* use named ui-modules * x-pack conversion is checked in so no need for the default uiModules export * Final removal of all export default in ui/public And turned on the eslint rule for that folder. * Moe import to top of file
…11351) * Remaining default to named conversions in ui/public (#11323) * use named ui-modules * x-pack conversion is checked in so no need for the default uiModules export * Final removal of all export default in ui/public And turned on the eslint rule for that folder. * Moe import to top of file * Convert remaining default exports in 5.x to get tests passing with the new rule
Some changes required to merge witth PRs: - elastic#11352 - elastic#11323
…ibana moved exports of certain ui modules. Check elastic/kibana#11323 for more info
Background: #11220
Switching over remaining modules to named and remove all default references.