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

eslint no-restricted-path false positive: bug caused by dir names that start with "index". #46544

Merged
merged 6 commits into from Oct 31, 2019

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Sep 25, 2019

Summary

Found this bug when writing code for #46537.

In current eslint setup, you cannot import from dirs whose name start with index like index_patterns in src/plugins dir even if they are in the same folder.

For example:

// src/plugins/data/server/plugin.ts
import { X } from './index_patterns'; // eslint error;
import { Y } from './hello_world'; // OK;

The cause of the problem is that traverseToTopFolder() returns the wrong folder when a folder name starts with 'index'.

You can check it out by running the script below:

const mm = require('micromatch');
const path = require('path');

const from = [
  'src/plugins/**/server/**/*',
  '!src/plugins/**/server/index*',
];

function traverseToTopFolder(src, pattern) {
  while (mm([src], pattern).length > 0) {
    const srcIdx = src.lastIndexOf(path.sep);
    src = src.slice(0, srcIdx);
  }
  return src;
}

console.log(traverseToTopFolder('src/plugins/data/server/plugin.ts', from));
console.log(traverseToTopFolder('src/plugins/data/server/index_patterns/index.ts', from));
console.log(traverseToTopFolder('src/plugins/data/server/dummy/index.ts', from));

Result:

src/plugins/data/server
src/plugins/data/server/index_patterns
src/plugins/data/server

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@lizozom lizozom self-assigned this Oct 29, 2019
@lizozom lizozom added v7.6.0 v8.0.0 Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Oct 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@lizozom
Copy link
Contributor

lizozom commented Oct 29, 2019

@elasticmachine merge upstream

@elasticmachine elasticmachine requested a review from a team as a code owner October 29, 2019 09:35
@lizozom
Copy link
Contributor

lizozom commented Oct 29, 2019

jenkins, test this

@lizozom lizozom added the Team:Operations Team label for Operations Team label Oct 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

.eslintrc.js Outdated Show resolved Hide resolved
@sainthkh
Copy link
Contributor Author

@spalger Added js and merged master.

@sainthkh sainthkh requested a review from spalger October 30, 2019 23:15
@lizozom
Copy link
Contributor

lizozom commented Oct 31, 2019

Jenkins test this

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom merged commit faa48d5 into elastic:master Oct 31, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Oct 31, 2019
…t start with "index". (elastic#46544)

* index* to index.{ts,tsx}

* Added test.

* Added invalid test.

* Added js.
lizozom pushed a commit that referenced this pull request Oct 31, 2019
…t start with "index". (#46544) (#49852)

* index* to index.{ts,tsx}

* Added test.

* Added invalid test.

* Added js.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants