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

@elasticmachine merge upstream

I'm cool with this approach as long as @elastic/kibana-operations is on board, especially considering we did the same thing in #35804 so it is not without precedent.

If for some reason this isn't viable, perhaps a next step would be looking further into Jason's idea about using querystring on the client & the built in node core querystring on the server.
(Edit: the main issue with querystring is that it is currently unmaintained)

This would be a much more involved change though -- so if dropping IE11 support in the future is inevitable, I prefer the simplicity of this solution.

I just tested this in IE11 and am still seeing syntax errors in ./node_modules/query-string/node_modules/strict-uri-encode/index.js. It seems like the problem is that the query-string module is not bundling its dependencies.

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: d6d33ed

History

• 💚 Build #29738 succeeded f58f6e61fbe8bf2a6b22fc1c7e125f22f5cee04c
• 💚 Build #29722 succeeded accde4838dd958212034867533c4d994779fc3a9
• 💛 Build #29528 was flaky c903fbfa4ae4842adf5f12c09637161ce0d5d585

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

Feedback from @elastic/kibana-operations was that we should not change the config like this to allow for compiling additional node_modules dependencies.

This leaves a few other options:

1. Select a different library. Options are limited here since we need something well-maintained, with the right APIs, which also has IE11 and TS support. Wondering if qs would be a possibility here, but would need to investigate further.
2. Revert the original PR. Functionally this would be pretty much the same as option 1, and we'd still need to get rid of the legacy ui/public/utils/query_string
3. Contribute changes back upstream. I don't think this is a possibility in this case as the maintainer had made it abundantly clear that they no longer intend to support older browsers as of v6 (see release notes).
4. Fork query-string until such a time as we don't need to support IE11. I don't love this idea because the only customization we require is having a single 340-line JS file run through babel, and duplicating the code creates an unnecessary maintenance burden.

After thinking about option 4, one other idea I had is creating an "empty" package in Kibana that basically exists to run node_modules/query-string through babel. This avoids the maintenance overhead of having duplicate code, without requiring us to change libraries (just import statements to use @kbn/query-string).

I put up a POC illustrating this. The only material difference between that technique and this PR is that babel will transpile up front during yarn kbn bootstrap. Otherwise it is basically the same thing.

I'm going to discuss pros & cons of our options with the team on Monday and will follow up here.

I chatted again with our friendly neighborhood ops team today. The conclusion was:

• We shouldn't be modifying the webpack configs, as mentioned above. Apparently Add exception to transpile normalize-url mode to es5 when bundled into the DLL #35804 was a special exception, but otherwise we've avoided adding these entirely.
• My POC for building the library in a package would work: POC - Export query-string as @kbn/query-string #58948
• @wylieconlon has a PR up to solve this as well: Switch back to querystring-browser library #58943

Wylie's PR basically reverts parts of the original PR, including the querystring webpack alias for querystring-browser. This way the lib can be imported consistently on the client and server. He also kept the changes that were made in kibana_utils.

IMO this is the better hybrid approach -- the POC I made works, but feels hacky and brittle. Whereas we know things we working well with querystring-browser before, even if it meant things were a bit more scattered.

Longer term, we should definitely still explore our options once it's safe to drop IE11 support -- maybe at that point it makes sense to go back to query-string. But shorter term, I'd rather keep things simple than fork query-string or build a special package as a workaround.