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

Remaining default to named conversions in ui/public #11323

Merged
merged 4 commits into from
Apr 20, 2017

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Apr 19, 2017

Background: #11220

Switching over remaining modules to named and remove all default references.

@stacey-gammon
Copy link
Contributor Author

jenkins, test this

@stacey-gammon stacey-gammon changed the title use named ui-modules Remaining default to named conversions in ui/public Apr 19, 2017
@stacey-gammon stacey-gammon requested a review from pickypg April 20, 2017 10:35
Copy link
Member

@pickypg pickypg left a 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';
Copy link
Member

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 imports?

Copy link
Contributor Author

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');
Copy link
Member

Choose a reason for hiding this comment

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

Should these become imports?

Copy link
Contributor Author

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.

Copy link
Contributor

@thomasneirynck thomasneirynck left a 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

@stacey-gammon stacey-gammon removed the request for review from simianhacker April 20, 2017 17:04
Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

🥇

@stacey-gammon stacey-gammon merged commit 9c37e6c into elastic:master Apr 20, 2017
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Apr 20, 2017
* 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
stacey-gammon added a commit that referenced this pull request Apr 20, 2017
…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
@stacey-gammon stacey-gammon deleted the use-named-uiModules branch April 21, 2017 12:44
hickeyma added a commit to hickeyma/kibana that referenced this pull request Apr 28, 2017
Some changes required to merge witth PRs:
 - elastic#11352
 - elastic#11323
sivasamyk added a commit to sivasamyk/logtrail that referenced this pull request Jul 8, 2017
…ibana moved exports of certain ui modules. Check elastic/kibana#11323 for more info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants