chore: 🤖 forbid import from ui/** #40537
Conversation
streamich
added
the
release_note:skip
streamich
requested review from
spalger and
mshustov
and removed request for
spalger
|
Pinging @elastic/kibana-operations |
|
Pinging @elastic/kibana-app-arch |
|
retest |
💚 Build Succeeded |
|
Before merging, let's make sure we finish the related discussion that was started in #39554 and #40032 This PR assumes a specific way of structuring shims that we haven't discussed previously. I'm not necessarily opposed to it, just want to be sure we have some sort of consensus before pushing something like this through. |
|
@lukeelmers This is a |
|
While getting ui/public imports out of the shims is our goal, I think that it will make migration harder and more cumbersome. It will prevent us from doing iterative work and will tie unnecessary work together: i.e.
|
streamich
changed the title
|
Agreed to use this rule in our weekly sync. Decided to use |
💚 Build Succeeded |
There was a problem hiding this comment.
Tested and verified the rule works as expected.
I tend to agree with @spalger that the changes in packages/kbn-eslint-plugin-eslint/rules/no_restricted_paths.js feel out of place, but I'm also not super familiar with the existing configuration, so I'll defer to @spalger or @restrry for any further comments on that topic.
If we are worried about the implementation here, IMO it would be fine if we needed to make tweaks to what we are checking for -- the only goal for this is to make peoples' lives easier if they opt to use a np_ready directory as part of their migration strategy (and it's going away at the end of the migration anyway).
| @@ -38,6 +38,7 @@ import { | |||
| EuiText, | |||
| } from '@elastic/eui'; | |||
|
|
|||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | |||
| import { SavedObjectAttributes } from 'src/core/server/saved_objects'; | |||
There was a problem hiding this comment.
I think src/** imports that used Webpack alias, like
import 'src/core/server/saved_objects';
where not covered previously, now this PR fixes it. Added a test for it, too.
💔 Build Failed |
|
ack: have a look tonight |
| @@ -8,6 +8,7 @@ import * as Rx from 'rxjs'; | |||
| import { Server } from 'hapi'; | |||
| import { Legacy } from 'kibana'; | |||
| import { KibanaConfig } from 'src/legacy/server/kbn_server'; | |||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | |||
There was a problem hiding this comment.
I was under the impression that this was the "correct" way to import mocks. Is there another approach we should take? Are mocks not meant to be used in this way at all?
There was a problem hiding this comment.
I guess it is still as "correct" as it was before. The difference is that before this PR @kbn/eslint/no-restricted-paths linter rule ignored Webpack aliases like src/.., now it processes them as well.
Maybe @elastic/kibana-platform could comment what is the "correct" way to import mocks/types? ^^
There was a problem hiding this comment.
yeah, unfortunately linter rule doesn't work with absolute paths yet. #36096
as a temporary solution you can change zones in .eslintrc
- '!src/core/server/mocks.ts',
+ '!src/core/server/mocks',
- '!src/core/public/mocks.ts',
+ '!src/core/public/mocks',I guess it is still as "correct" as it was before.
right
|
retest |
💔 Build Failed |
| @@ -18,6 +18,7 @@ | |||
| */ | |||
|
|
|||
| import { PluginInitializerContext } from 'kibana/public'; | |||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | |||
| import { npSetup, npStart } from 'ui/new_platform'; | |||
There was a problem hiding this comment.
should we allow importing from ui/new_platform? if yes you can add !ui/new_platform in zone declaration inside .eslintrc
| @@ -6,6 +6,7 @@ | |||
|
|
|||
| import getPort from 'get-port'; | |||
| import { resolve } from 'path'; | |||
| // eslint-disable-next-line @kbn/eslint/no-restricted-paths | |||
There was a problem hiding this comment.
and tests may import from anywhere. we also can exclude them from the check
streamich
changed the title
💚 Build Succeeded |
streamich
added
the
Team:Security
|
Pinging @elastic/kibana-security |
* chore: 🤖 forbid importing from ui/** from within shims * chore: 🤖 allow importing platfrom in dashboard embeddable sihm * chore: 🤖 use np_ready folder, improve rule * chore: 🤖 update ESLint error message * feat: 🎸 improve eslint plugin rule * test: 💍 add tests for restricted paths linter rule * test: 💍 add ESLint package to CI testing suite * chore: 🤖 fix linter errors
Summary
np_readyfolder to importui/**.src/core/**, whensrc/**Webpack alias is used.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriatelyThis includes a feature addition or change that requires a release note and was labeled appropriately