Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Do not serve legacy JS when serving a Kibana Platform application #61011
Do not serve legacy JS when serving a Kibana Platform application #61011
Changes from 21 commits
7af9555
76f0ee8
a37876f
4aba2c5
ef11745
79c1a43
8cb723b
21c3225
b498623
90938b7
32c572a
f9cc17d
af039ee
6834f59
c7c6738
05ba7b1
9b6d30a
5d893d6
329c044
fb44467
1fd2e06
237e2c5
6f648b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Are we planning to have additional
entry
type bundles? If not, wouldn'tcore
be more explicit?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could imagine the login and logout pages being "entry" type bundles. The inclusion of plugins in those bundles has caused a number of issues over the years and it seems there needs not be a whole "platform" loaded to render those two pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, maybe it'd be better if the navigation after login was a SPA-style nav to the next app, which would be instant. Either way, I'm sort of indifferent here. I just used
entry
to make it more clear how this bundle could be used in a browser.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all about making login instant, it's really logout that has caused the most issues and it's also one of those pages that should really be as lightweight as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's feasible. That would add a limitation that security plugin cannot use other plugins API (licensing, for example) on
login
/logout
pages. Also, it's already working with bootstrapping all the KP plugins.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There have been a handful of nasty race conditions caused by bootstrapping all the plugins on the logout page, which have lead to security vulnerabilities (all unshipped I believe), and having all that extra logic on the logout page currently serves no purpose.
I'm on board with "if it ain't broke, don't fix it", but I think this has broken enough times that we should look for solutions. Especially since the bugs have historically been "I clicked logout, and it said I was logged out, but when I refresh the login page I am logged in"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be passing css to the sass-loader, should we? If the file should be parsed as sass I think we should change the extension to scss and not automatically parse it as scss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I added this in my first POC iterations and forgot to remove.