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

Fix import causing Kibana to crash in IE11. #52248

Merged
merged 3 commits into from
Dec 9, 2019

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Dec 5, 2019

Closes #50927

A regression was caused by the changes in #50182, where some client-side code appeared to be indirectly importing some server-side utilities from the data plugin src/plugins/data/server.

This caused a bunch of additional code to be bundled in the commons.bundle.js, which when executed from IE11, caused a syntax error. (I actually never got to what the specific syntax error was in my investigation, as I was primarily focused on getting rid of the server-side code that was being bundled with everything).

Turns out that some of the Canvas common functions (in this case, saved_map) which are loaded from the browser, are relying on a buildEmbeddableFilters method which is located in the Canvas server lib. This imports some stuff from src/plugins/data/server, which is how the code ended up in the common bundle to begin with.

That said, I believe the changes introduced in #50182 were actually correct: server files should be able to import static code from other server files. The temporary workaround here was to import from src/plugins/data/common, but the real fix is figuring out whether the pieces of buildEmbeddableFilters needed by the client-side code could be split out. I've created #52343 to track this work.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Dosant
Copy link
Contributor

Dosant commented Dec 5, 2019

Awesome! Thanks!

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers lukeelmers added the release_note:skip Skip the PR/issue when compiling release notes label Dec 5, 2019
@lukeelmers lukeelmers marked this pull request as ready for review December 5, 2019 21:39
@lukeelmers lukeelmers requested a review from a team as a code owner December 5, 2019 21:39
@lukeelmers
Copy link
Member Author

lukeelmers commented Dec 5, 2019

Also cc @elastic/kibana-platform -- If it were possible to have linter rules to catch these sorts of bad imports that inadvertently cross server/client boundaries, it would help eliminate a whole category of bugs we have been seeing during the migration process. Not sure how feasible that would be to implement, but wanted to mention it here.

@lukeelmers lukeelmers self-assigned this Dec 5, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked that can now launch Kibana in IE11
Screenshot 2019-12-06 at 11 25 37

@mshustov
Copy link
Contributor

mshustov commented Dec 6, 2019

@lukeelmers as I can see x-pack/legacy/plugins/canvas/server/lib/build_embeddable_filters.ts is supposed to contain server code. It's rather interesting why it's imported from files under the common folder.
You can add this check in eslintrc file

{
  target: ['**/common/**/*'],
  from: ['**/server/**/*', '**/public/**/*'],
  errorMessage: 'Common code cannot import env-dependent code',
},

and it shows 265 problems in the current codebase.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE11 is happy again 🌻 !

@joshdover
Copy link
Contributor

Also cc @elastic/kibana-platform -- If it were possible to have linter rules to catch these sorts of bad imports that inadvertently cross server/client boundaries, it would help eliminate a whole category of bugs we have been seeing during the migration process. Not sure how feasible that would be to implement, but wanted to mention it here.

Just put up a PR here: #52447
We'll have to see how many violations we already have on CI, couldn't get eslint to finish in a reasonable time on my machine.

@pgayvallet
Copy link
Contributor

refers to #49053

Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@kertal
Copy link
Member

kertal commented Dec 9, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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

@lukeelmers lukeelmers removed the review label Dec 9, 2019
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kibana throws an error after logging in at IE11
9 participants