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

[linting] Enforce relative paths for X-Pack modules #34349

Merged
merged 4 commits into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@tylersmalley
Copy link
Member

tylersmalley commented Apr 2, 2019

Breaking up #32722 by first creating a TSLint rule to enforce relative imports, allowing us to remove X-Pack as a node module.

@tylersmalley tylersmalley requested a review from elastic/apm-ui as a code owner Apr 2, 2019

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 2, 2019

Add module-migration support for toRelative
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@elasticmachine

This comment was marked as outdated.

Copy link

elasticmachine commented Apr 2, 2019

tslint fixes
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

@tylersmalley tylersmalley force-pushed the tylersmalley:xpack-all-relative branch from 64abab8 to f119214 Apr 2, 2019

const moduleId = node.moduleSpecifier.text;
const mapping = this.options.find(
mapping => mapping.from === moduleId || mapping.from.startsWith(moduleId + '/')

This comment has been minimized.

Copy link
@tylersmalley

tylersmalley Apr 2, 2019

Author Member

This was incorrect, fixed to check the module being imported instead of the lint configuration.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 2, 2019

@spalger

spalger approved these changes Apr 2, 2019

Copy link
Member

spalger left a comment

Couple little things, but LGTM

Show resolved Hide resolved src/dev/tslint/rules/moduleMigrationRule.js Outdated
Show resolved Hide resolved x-pack/tslint.yaml Outdated

tylersmalley added some commits Apr 2, 2019

Move configuration to root
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Simplify root definition
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley

This comment has been minimized.

Copy link
Member Author

tylersmalley commented Apr 2, 2019

@elastic/apm-ui already approved the changes in #32722 where this was first introduced so I won't block on a review here.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 2, 2019

@spalger

spalger approved these changes Apr 2, 2019

@tylersmalley tylersmalley merged commit 1b95bed into elastic:master Apr 2, 2019

2 checks passed

CLA All commits in pull request signed
Details
kibana-ci Build finished.
Details

tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Apr 2, 2019

[linting] Enforce relative paths for X-Pack modules (elastic#34349)
* Add module-migration support for toRelative

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley added a commit that referenced this pull request Apr 2, 2019

[linting] Enforce relative paths for X-Pack modules (#34349)
* Add module-migration support for toRelative

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>

tylersmalley added a commit that referenced this pull request Apr 2, 2019

[linting] Enforce relative paths for X-Pack modules (#34349) (#34370)
* Add module-migration support for toRelative

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@sqren

This comment has been minimized.

Copy link
Member

sqren commented Apr 5, 2019

@tylersmalley Good job adding the lint rule to require relative paths 👍
I've noticed that when auto-importing in vscode, the aliased path is still used, and I therefore have to manually specify the relative path. This defeats the purpose of auto-importing, and would be great if we could get it to suggest the relative path instead.

Screenshot 2019-04-05 at 11 21 06

I suspect that somewhere we still specify x-pack/plugins/{plugin-name} as a valid alias, and vscode is picking that up. I tried removing the paths in this tsconfig.json but that didn't make a difference.

Any clue?

@sqren

This comment has been minimized.

Copy link
Member

sqren commented Apr 5, 2019

Follow-up: I was able to fix this by updating vscode's settings.json:

"typescript.preferences.importModuleSpecifier": "relative"

Not sure if this can be fixed on a more general level, but for now this works well for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.