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

Add no default exports custom rule #10981

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Mar 31, 2017

Abide by the decision made here #8641 and introduce a new eslint rule.

For now I am only introducing it at kibana/src/core_plugins/kibana/public. It's going to be a large chunk of work to convert everything over but at least this covers a portion of the code base, while I work on incrementally adding it to other parts.

@jbudz
Copy link
Member

jbudz commented Mar 31, 2017

Thoughts on making this part of the base config https://github.com/elastic/eslint-config-kibana? And maybe we could explicitly override it to off until some automation can pick up the rest.

@stacey-gammon
Copy link
Contributor Author

Ah, perfect @jbudz, wasn't aware of that. Yes, seems appropriate to add there.

@stacey-gammon
Copy link
Contributor Author

@jbudz I take it back, I seem to be having trouble making the rule part of that package, and then including it in kibana. Maybe I am doing something wrong. Here is some of my configuration:

screen shot 2017-03-31 at 3 04 47 pm

My suspicion is it's because my rule is a local, nested npm module, referred to via a relative file, not a true, hosted npm module.

Unless you have any suggestions, I'm going to keep it local to kibana for now, since I was able to get that working.

@stacey-gammon stacey-gammon force-pushed the es-lint-no-default-exports branch 4 times, most recently from 465c0fc to 0546a7d Compare March 31, 2017 19:24
@stacey-gammon
Copy link
Contributor Author

Jenkins error:

Running "karma:ciShard-1" (karma) task
managr    log   [19:30:57.365] [error][status][ui settings] Status changed from uninitialized to red - UI Settings requires the elasticsearch plugin
31 03 2017 19:30:57.504:INFO [karma]: Karma v1.2.0 server started at http://localhost:9876/
31 03 2017 19:30:57.505:INFO [launcher]: Launching browser Chrome with unlimited concurrency
31 03 2017 19:30:57.511:INFO [launcher]: Starting browser Chrome
31 03 2017 19:30:58.343:INFO [Chrome 57.0.2987 (Linux 0.0.0)]: Connected on socket /#b2tURJnRUiqlIsJJAAAA with id 15889894
Chrome 57.0.2987 (Linux 0.0.0) LOG: 'ready to load tests for shard 1 of 4'

Chrome 57.0.2987 (Linux 0.0.0) ERROR: 'Error: Uncaught TypeError: __webpack_require__(...).register is not a function (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:194183)
    at window.onerror (http://localhost:5610/bundles/tests.bundle.js?shards=4&shard_num=1:72332:23)'


Chrome 57.0.2987 (Linux 0.0.0): Executed 0 of 0 SUCCESS (0 secs / 0 secs)
Chrome 57.0.2987 (Linux 0.0.0): Executed 0 of 0 ERROR (0.003 secs / 0 secs)
Warning: Task "karma:ciShard-1" failed.� Use --force to continue.

hmmm...
jenkins, test this

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

Maybe move the custom-eslint-rules to a packages/ directory? I think that's a nice default from https://lernajs.io/ to also use here (aka whenever we build packages that Kibana itself relies on, put them in packages/ unless we create separate repos for them.)

We should perhaps also npm publish it when we merge, so that xpack can rely on the same rule.

(Longer term we should maybe pull the Kibana eslint config into this repo too)

@stacey-gammon stacey-gammon requested review from jbudz and removed request for spalger and tylersmalley April 4, 2017 11:40
@stacey-gammon
Copy link
Contributor Author

@kjbekkelund Took your suggestion for the packages dir and moved the new folder in there.

As for npm publish, that sounds good to me as a second step, though I haven't npm published anything before so I'm not sure of the steps.

(Longer term we should maybe pull the Kibana eslint config into this repo too)

I think the preferred place for this new rule to go was in the shared config, but I was having issues getting it to run correctly in there. For the sake of time, decided to keep it in here. So sounds like the separate eslint repo is preferred? I don't have an opinion on the matter. cc @spalger who initially suggested I move this rule into that package.

@stacey-gammon stacey-gammon force-pushed the es-lint-no-default-exports branch from 8ad743f to 5e3619a Compare April 4, 2017 18:22
@stacey-gammon
Copy link
Contributor Author

Build error due to S3/amazon issue

ERROR: Failed to upload files
com.amazonaws.SdkClientException: Unable to execute HTTP request: java.lang.RuntimeException: Unexpected error: java.security.InvalidAlgorithmParameterException: the trustAnchors parameter must be non-empty
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.executeHelper(AmazonHttpClient.java:972)
	at com.amazonaws.http.AmazonHttpClient$RequestExecutor.doExecute(AmazonHttpClient.java:676)

jenkins, test this.

@stacey-gammon stacey-gammon force-pushed the es-lint-no-default-exports branch from 5e3619a to 84b108a Compare April 11, 2017 15:51
@stacey-gammon
Copy link
Contributor Author

@kjbekkelund Any last feedback or is this good to submit (once 5.4 branch is created so I don't cause any 5.4 last minute delays)?

"engines": {
"node": ">=0.10.0"
},
"license": "ISC"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file might be mostly copied, e.g. requireindex, mocha. I think this should be a minimal npm package.json, if possible, and only specify name, version, and whatever deps are actually needed (if any).

(Could maybe keep the description and make it Custom ESLint rules for Kibana)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, great catch. Indeed much of it was copied. :) 👍

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

Other than the package.json it LGTM

@stacey-gammon stacey-gammon force-pushed the es-lint-no-default-exports branch from 84b108a to 1c966df Compare April 12, 2017 00:12
@stacey-gammon stacey-gammon merged commit 5aea4ff into elastic:master Apr 12, 2017
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Apr 12, 2017
* Introduce a custom kibana estlint rule for no default exports

Turn it on only at the level of /kibana/src/core_plugins/public/ for
now, just to keep the PR sizes manageable.

* Don't call functions directly on the import

* Create a packages dir and move the custom rule in there

* Remove copied package.json portions, use minimal info necessary
stacey-gammon added a commit that referenced this pull request Apr 13, 2017
* Introduce a custom kibana estlint rule for no default exports

Turn it on only at the level of /kibana/src/core_plugins/public/ for
now, just to keep the PR sizes manageable.

* Don't call functions directly on the import

* Create a packages dir and move the custom rule in there

* Remove copied package.json portions, use minimal info necessary
@stacey-gammon stacey-gammon deleted the es-lint-no-default-exports branch April 13, 2017 13:09
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