Skip to content

Fix import causing Kibana to crash in IE11.#52248

Merged
lukeelmers merged 3 commits intoelastic:masterfrom
lukeelmers:fix/ie11
Dec 9, 2019
Merged

Fix import causing Kibana to crash in IE11.#52248
lukeelmers merged 3 commits intoelastic:masterfrom
lukeelmers:fix/ie11

Conversation

@lukeelmers
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers commented Dec 5, 2019 •
edited
Loading

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
Copy Markdown
Contributor

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

@Dosant
Copy link
Copy Markdown
Contributor

Dosant commented Dec 5, 2019

Awesome! Thanks!

@lukeelmers lukeelmers mentioned this pull request Dec 5, 2019
Closed
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Dec 5, 2019

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
Copy Markdown
Contributor Author

lukeelmers commented Dec 5, 2019 •
edited
Loading

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
Copy Markdown
Contributor

elasticmachine commented Dec 5, 2019

💚 Build Succeeded

Dosant
Dosant approved these changes Dec 6, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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.

kertal
kertal approved these changes Dec 6, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

joshdover commented Dec 6, 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.

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
Copy Markdown
Contributor

pgayvallet commented Dec 7, 2019

refers to #49053

alexwizp
alexwizp approved these changes Dec 9, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

kertal commented Dec 9, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Dec 9, 2019

💚 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
crob611
crob611 approved these changes Dec 9, 2019
View reviewed changes
Copy link
Copy Markdown
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

@lukeelmers lukeelmers merged commit de4269f into elastic:master Dec 9, 2019
@lukeelmers lukeelmers deleted the fix/ie11 branch December 9, 2019 23:29
This was referenced Dec 9, 2019
Merged
Merged
lukeelmers added a commit that referenced this pull request Dec 10, 2019
@lukeelmers
Fix import causing Kibana to crash in IE11. (#52248) (#52583)
9940333
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kertal kertal kertal approved these changes

@Dosant Dosant Dosant approved these changes

@alexwizp alexwizp alexwizp approved these changes

+1 more reviewer

@crob611 crob611 crob611 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lukeelmers lukeelmers

Labels

Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Kibana throws an error after logging in at IE11

9 participants

@lukeelmers @elasticmachine @Dosant @mshustov @joshdover @pgayvallet @kertal @crob611 @alexwizp