disallow external imports to src/core/utils #64852
Conversation
spalger
force-pushed
the
fix/remove-imports-for-core-utils
branch
3 times, most recently
from
c4b2c90 to
4c86c5e
Compare
src/core/server/index.ts
Outdated
There was a problem hiding this comment.
@joshdover I'm surprised we have this in utils. Shouldn't we move it to the application or another service instead? utils are meant to provide functionality extending the language standard library.
There was a problem hiding this comment.
I believe this is utils due to problems with importing from server into public. We can't use the server/types workaround since this is a value. Long-term we could fix this by either leveraging a common directory or the organize by domain proposal for core #52182
There was a problem hiding this comment.
Ok, I think we can get rid of the server-side use-cases when all plugin migrate to KP most plugins migrate to KP and we can inline values in the remaining cases.
There was a problem hiding this comment.
DEFAULT_APP_CATEGORIES 'util' was introduced during the grouped nav PR, because these values needed to be accessed by the legacy apps registrations that are performed on the server-side. I agree with @restrry that once we only have a small amount of legacy app registrations, moving this back to client-side and using inline values for the legacy apps would be an acceptable solution.
spalger
force-pushed
the
fix/remove-imports-for-core-utils
branch
2 times, most recently
from
2e768c5 to
1331b8d
Compare
spalger
force-pushed
the
fix/remove-imports-for-core-utils
branch
from
1331b8d to
ccdb8e7
Compare
|
@elasticmachine merge upstream |
…ports-for-core-utils # Conflicts: # x-pack/legacy/plugins/canvas/index.js
spalger
added
release_note:skip
|
Pinging @elastic/kibana-operations (Team:Operations) |
|
@elasticmachine merge upstream |
chrisronline
left a comment
chrisronline
left a comment
There was a problem hiding this comment.
LGTM for stack monitoring
nchaulet
left a comment
nchaulet
left a comment
There was a problem hiding this comment.
Ingest management changes LGTM
jasonrhodes
left a comment
jasonrhodes
left a comment
There was a problem hiding this comment.
Infra and Ingest Manager changes LGTM, just simple path changes 👍
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
Scanned all the code changes, LGTM. Left a question to confirm my understanding.
| // https://www.typescriptlang.org/docs/handbook/advanced-types.html#exhaustiveness-checking | ||
| export function assertNever(x: never): never { | ||
| throw new Error(`Unexpected object: ${x}`); | ||
| } |
There was a problem hiding this comment.
Why not import this assertNever just like all the otherrs? Is it because it's in common?
There was a problem hiding this comment.
Yep, there isn't currently a common import for src/core and it was just so simple.
|
@elasticmachine merge upstream |
|
Pinging @elastic/apm-ui (Team:apm) |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
18 participants
While working on #64843 I noticed a number of imports for the
src/core/utilsmodule, which the platform team didn't expect to be used externally, so I've re-exported portions of that module fromsrc/core/serverandsrc/core/public, updated all uses, and added an eslint config to validate we don't add moresrc/core/utilsimports down the road.