Add lint rule to prevent server code being imported into client #52447

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

@@ -31,7 +31,7 @@ import {
 import { AuthenticationResult } from './authentication_result';
 import { DeauthenticationResult } from './deauthentication_result';
 import { Tokens } from './tokens';
-import { SessionInfo } from '../../public/types';
+import { SessionInfo } from '../../public';


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.

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 f25494d
💔 Build #15220 failed a5394fc
💔 Build #15188 failed 7015c36
💔 Build #14972 failed ca8e5fd

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)

…tic#52447)

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 Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc 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 review from a team as code owners 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

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

timductive pushed a commit to timductive/kibana that referenced this pull request Dec 16, 2019

Add lint rule to prevent server code being imported into client (elas…

eaebe2f

…tic#52447)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint rule to prevent server code being imported into client #52447

Add lint rule to prevent server code being imported into client #52447

joshdover commented Dec 6, 2019 •

edited

Loading

joshdover commented Dec 6, 2019

pgayvallet commented Dec 7, 2019

elasticmachine commented Dec 9, 2019

kobelb Dec 9, 2019

joshdover Dec 9, 2019

mshustov Dec 9, 2019

joshdover Dec 9, 2019

smith left a comment

streamich left a comment •

edited

Loading

joshdover commented Dec 9, 2019

streamich commented Dec 9, 2019

jfsiii commented Dec 9, 2019

joshdover commented Dec 11, 2019

mikecote left a comment

walterra left a comment

lizozom left a comment •

edited

Loading

joshdover commented Dec 13, 2019

elasticmachine commented Dec 13, 2019

joshdover commented Dec 13, 2019

Add lint rule to prevent server code being imported into client #52447

Add lint rule to prevent server code being imported into client #52447

Conversation

joshdover commented Dec 6, 2019 • edited Loading

Summary

Checklist

For maintainers

joshdover commented Dec 6, 2019

pgayvallet commented Dec 7, 2019

elasticmachine commented Dec 9, 2019

kobelb Dec 9, 2019

Choose a reason for hiding this comment

joshdover Dec 9, 2019

Choose a reason for hiding this comment

mshustov Dec 9, 2019

Choose a reason for hiding this comment

joshdover Dec 9, 2019

Choose a reason for hiding this comment

smith left a comment

Choose a reason for hiding this comment

streamich left a comment • edited Loading

Choose a reason for hiding this comment

joshdover commented Dec 9, 2019

streamich commented Dec 9, 2019

jfsiii commented Dec 9, 2019

joshdover commented Dec 11, 2019

mikecote left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

lizozom left a comment • edited Loading

Choose a reason for hiding this comment

joshdover commented Dec 13, 2019

elasticmachine commented Dec 13, 2019

💚 Build Succeeded

History

joshdover commented Dec 13, 2019

joshdover commented Dec 6, 2019 •

edited

Loading

streamich left a comment •

edited

Loading

lizozom left a comment •

edited

Loading