-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[New platform] Restrict import from core&plugin internals for js files #33697
[New platform] Restrict import from core&plugin internals for js files #33697
Conversation
Pinging @elastic/kibana-platform |
💔 Build Failed |
Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder.
50ea2d2
to
5e1510a
Compare
return path.join(__dirname, 'files', relativePath); | ||
} | ||
|
||
ruleTester.run('@kbn/eslint/no-restricted-paths', rule, { |
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.
@spalger I cannot see "test" script in https://github.com/elastic/kibana/blob/master/packages/kbn-eslint-plugin-eslint/package.json
how we run tests for the package? I did use `node path/to/test' to run tests manually
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.
node scripts/mocha packages/kbn-eslint-plugin-eslint/rules/__tests__/no_restricted_paths.js
💔 Build Failed |
💚 Build Succeeded |
create(context) { | ||
const options = context.options[0] || {}; | ||
const zones = options.zones || []; | ||
const basePath = process.cwd(); |
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.
Can we make this configurable? We use .js
files for eslint config so we can provide the absolute root for all files, or even require that zone.target
/zone.target
are absolute.
.eslintrc.js
Outdated
zones: [ | ||
{ | ||
// when tslint is removed we will check *.ts files as well | ||
target: './src/legacy/.*js$', |
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'm not opposed to limiting to .js files, but do you think it's necessary? ESLint will only process the files .js files, and this will automatically apply to .ts files as soon as we get them in here without this limitation.
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.
didn't want to force #33826 to comment/fix all problem places in *.ts
files.
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.
We're going to limit all existing rules to .js files in that PR, so I wouldn't worry about it.
@@ -0,0 +1,149 @@ | |||
/* eslint-disable @kbn/eslint/require-license-header */ | |||
/* @notice |
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.
Since we don't ship this code in the distributable I don't think we need to include the @notice
, do you agree @epixa?
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.
Hm, I'd have to reacquaint myself with how we handle that annotation. I don't think it makes sense to ship this package's license info in our distributions' notice file. I do still think we should be automatically checking it's license, albeit against our dev-acceptable list. In general, I'd rather ship a dependency's info in our notice file by accident than accidentally leave out a dependency we should have included.
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.
This tag doesn't cause the licence to be checked, that's something we do "manually" by committing the code to the repo. This tag is only for generating the notice file in the build, so I think we can get rid of it.
I think I saw a reason for this, but I can't track it down: can we contribute the change upstream? |
@epixa I was going but realized it could take some time to have it merged. At least there are several PRs are waiting for CR import-js/eslint-plugin-import#1238 |
@spalger switched to |
💚 Build Succeeded |
} | ||
|
||
function resolveBasePath(basePath) { | ||
return path.isAbsolute(basePath) ? basePath : path.relative(process.cwd(), basePath); |
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.
path.resolve(process.cwd(), basePath)
is going to return a relative path... I don't think that's what you're trying to do here.
create(context) { | ||
const options = context.options[0] || {}; | ||
const zones = options.zones || []; | ||
const basePath = options.basePath ? resolveBasePath(options.basePath) : process.cwd(); |
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.
Why not just require options.basePath
? I think it would make the behavior of this rule more explicit and easier to reason about.
const basePath = options.basePath;
if (!basePath || !isAboslute(basePath)) {
throw new Error('basePath option must be specified and must be absolute')
}
💚 Build Succeeded |
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.
LGTM
elastic#33697) * restrict import from core&plugin internals * Fork import/no-restricted-paths and add allowSameFolder option Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder. * update notices * add basePath option * support glob pattern instead of reagexp * remove @notice, make basePath required
#33697) (#34139) * restrict import from core&plugin internals * Fork import/no-restricted-paths and add allowSameFolder option Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define 'from: plugins/*/server/' the rule will report all relative imports in the same folder as well. To fix this problem we added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder. * update notices * add basePath option * support glob pattern instead of reagexp * remove @notice, make basePath required
import/no-restricted-paths supports restrictions by zone mechanism.
Our use case requires to restrict imports from plugin folders, which names are unknown for us yet. We cannot use 'import/no-restricted-paths' in the current state, because if we define
from: plugins/*/server/
the rule will report all relative imports in the same folder as well. To fix this problem I added another option 'allowSameFolder' that makes the rule to ignore imports in the same folder.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 IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately