Add lint rule to prevent server code being imported into client by joshdover · Pull Request #52447 · elastic/kibana

joshdover · 2019-12-06T22:20:55Z

Summary

Fixes #49053

This adds new configurations for the no-restricted-paths eslint rule to prevent:

Importing any server-side modules into any modules that are not in a server/ directory.
- This includes common directories often used by plugins. Necessary to ensure that modules containing shared code do not import anything that should be exclusively used on the server.
- This helps avoid issues where server code inadvertently ends up in a Webpack bundle that breaks the front-end. When these problems happen it is often hard to understand why, and this rule should help.
Importing any code from ui/* or uiExports/* into New Platform plugins.
- These modules will be deleted in the future and should not be supported in New Platform plugins.

Checklist

Use ~~strikethroughs~~ to 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
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For 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

joshdover · 2019-12-06T22:46:50Z

Didn't get this rule quite right, but now that I have some failures I can play around with it. Will circle back next week.

pgayvallet · 2019-12-07T09:49:50Z

Fixes #49053 btw

.eslintrc.js

elasticmachine · 2019-12-09T18:37:56Z

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

kobelb · 2019-12-09T18:56:31Z

x-pack/plugins/security/server/authentication/authenticator.ts

FWIW, I like the changes you made. Did the lint rule require these changes?

Yes it does. If you don't want this to be part of the public contract, we can move this up into a shared types file that only security can import from.

BTW shouldn't we ban public code to be imported from server as well? I'm concern about cases when a plugin accesses browser specific API and can throw on the server side

I'm not against that, was just focusing this change on fixing the Webpack bundling problems. It looks like there are more violations of this pattern, let me see about doing that change in a separate PR.

.eslintrc.js

smith

APM stuff looks good!

streamich

Shall we also add rules that:

/src/plugins/*/server/**/* should not be able to import from /src/plugins/*/public/**/*
/src/plugins/*/public/**/* should not be able to import from /src/plugins/*/server/**/*
/src/plugins/*/common/**/* should not be able to import from server, nor public

joshdover · 2019-12-09T22:44:54Z

Shall we also add rules that:

/src/plugins/*/server/**/* should not be able to import from /src/plugins/*/public/**/*

As mentioned above, I will do this in a separate change as there are more things to fix here and this is less urgent.

/src/plugins/*/public/**/* should not be able to import from /src/plugins/*/server/**/*

/src/plugins/*/common/**/* should not be able to import from server, nor public

Both of these are already covered in this PR. Nothing may import from server that is not also a server directory. This eliminates the immediate problem where Webpack bundles get broken.

streamich · 2019-12-09T23:06:48Z

I see, thanks, every time I get confused by from and target meaning in that ESLint rule.

jfsiii · 2019-12-09T23:34:16Z

Is this only for NP plugins? It seems useful for many. Anecdotally, I've done this twice in the last few months in an x-pack/legacy plugin and would love some protection from myself :)

Just curious, nothing to hold up this PR.

lukeelmers · 2019-12-09T23:54:22Z

Is this only for NP plugins? It seems useful for many.

++ This is what I was wondering.

Would be great if the imports-from-server part could be run on legacy plugins too. For example, these updates would not have prevented this regression which was fixed recently, because it was in x-pack/legacy/plugins.

Of course, ui/ and uiExports/ should probably still be allowed in the legacy world, but is there any reason why we wouldn't want to enforce the server imports bit?

joshdover · 2019-12-11T21:38:28Z

Added a rule to cover legacy plugins as well. There are a lot more violations here, some of which were trivial to fix, others which are not. For the more complex ones I have added eslint-ignore lines. These should be cleaned up when migrating to the New Platform.

mikecote

Stack services changes LGTM 👍 Created #52896 for us to change Task Manager's folder structure.

walterra

ML changes LGTM

lizozom

@streamich comments aside, the changes to @elastic/kibana-app-arch files are LGTM

joshdover · 2019-12-13T16:41:11Z

@elasticmachine merge upstream

elasticmachine · 2019-12-13T18:19:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: da66f37

History

💚 Build #15277 succeeded a9816d5
💚 Build #15237 succeeded f25494dd792ecca8262cf2d68cca580c0eb7f9cd
💔 Build #15220 failed a5394fc5cd0d0ffcd949aca1c1b6a1162f0f7c6a
💔 Build #15188 failed 7015c36
💔 Build #14972 failed ca8e5fddc5b585739b271ad39996919a214e6d81

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

joshdover · 2019-12-13T18:29:56Z

Opened #53021 to track adding a lint rule to forbid importing in the other direction (client code -> server code)

joshdover mentioned this pull request Dec 6, 2019

Fix import causing Kibana to crash in IE11. #52248

Merged

mshustov reviewed Dec 7, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

mshustov reviewed Dec 7, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

joshdover force-pushed the np/server-client-imports branch 4 times, most recently from 9800f40 to ab87325 Compare December 9, 2019 18:35

joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.6.0 labels Dec 9, 2019

joshdover marked this pull request as ready for review December 9, 2019 18:38

joshdover requested review from a team as code owners December 9, 2019 18:38

kobelb reviewed Dec 9, 2019

View reviewed changes

mshustov approved these changes Dec 9, 2019

View reviewed changes

mshustov reviewed Dec 9, 2019

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

joshdover force-pushed the np/server-client-imports branch from ab87325 to a0f830f Compare December 9, 2019 20:33

joshdover requested a review from a team December 9, 2019 20:33

joshdover requested a review from a team as a code owner December 9, 2019 20:33

smith approved these changes Dec 9, 2019

View reviewed changes

streamich reviewed Dec 9, 2019

View reviewed changes

joshdover force-pushed the np/server-client-imports branch from a0f830f to 2a10c37 Compare December 9, 2019 21:12

joshdover requested a review from a team December 11, 2019 21:35

joshdover requested a review from a team as a code owner December 11, 2019 21:35

mikecote mentioned this pull request Dec 12, 2019

Change Task Manager folder structure to include a "server" folder #52896

Closed

mikecote approved these changes Dec 12, 2019

View reviewed changes

walterra approved these changes Dec 12, 2019

View reviewed changes

joshdover added 4 commits December 12, 2019 10:38

Add lint rule to prevent server code being imported into client

f36c2d7

Fix bad imports

b75eb52

Add server imports check to legacy plugins

fdd6f28

Fix bad legacy imports

7015c36

joshdover force-pushed the np/server-client-imports branch 2 times, most recently from a5394fc to f25494d Compare December 12, 2019 19:28

Fix typings

a9816d5

joshdover force-pushed the np/server-client-imports branch from f25494d to a9816d5 Compare December 12, 2019 21:59

rudolf approved these changes Dec 13, 2019

View reviewed changes

lizozom approved these changes Dec 13, 2019

View reviewed changes

Merge branch 'master' into np/server-client-imports

da66f37

lizozom mentioned this pull request Dec 13, 2019

Embeddable examples on the platform and included with --run-examples flag #52111

Merged

joshdover merged commit ec2134d into elastic:master Dec 13, 2019

joshdover deleted the np/server-client-imports branch December 13, 2019 18:26

joshdover mentioned this pull request Dec 13, 2019

[7.x] Add lint rule to prevent server code being imported into client (#52447) #53019

Merged

lizozom mentioned this pull request Dec 16, 2019

move management registry to new platform #53020

Merged

jfsiii mentioned this pull request Dec 16, 2019

[EPM] Don't share CreateFakeDatasource type #53068

Merged

jfsiii mentioned this pull request Dec 30, 2019

[EPM] Code in 'common' directories shouldn't import server code #53854

Merged

This was referenced Jan 3, 2020

[Fleet] Code in 'common' directories shouldn't import server code #53938

Merged

[Fleet] Remove server code from common folder in ingest #53969

Merged

Conversation

joshdover commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

joshdover commented Dec 6, 2019

Uh oh!

pgayvallet commented Dec 7, 2019

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Dec 9, 2019

Uh oh!

kobelb Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

joshdover Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

mshustov Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

joshdover Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smith left a comment

Choose a reason for hiding this comment

Uh oh!

streamich left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshdover commented Dec 9, 2019

Uh oh!

streamich commented Dec 9, 2019

Uh oh!

jfsiii commented Dec 9, 2019

Uh oh!

lukeelmers commented Dec 9, 2019

Uh oh!

joshdover commented Dec 11, 2019

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

lizozom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshdover commented Dec 13, 2019

Uh oh!

elasticmachine commented Dec 13, 2019

💚 Build Succeeded

History

Uh oh!

joshdover commented Dec 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

joshdover commented Dec 6, 2019 •

edited

Loading

streamich left a comment •

edited

Loading

lizozom left a comment •

edited

Loading