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

build: enable linking charts library to kibana #1164

Merged
merged 13 commits into from
May 27, 2021

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented May 20, 2021

Summary

Adds a new command line aid for linking charts library to kibana.

Important features:

  • Single command runs everything
  • Cleanup is simple
  • Both charts and kibana are restored to original state
  • Handles many non-happy paths, but not all 😢

Entry command:

yarn link:kibana [--debug]

You will be prompted to Link, Unlink or Watch mode.

Link - Starts linking process and ends in watch mode to update built files
Unlink - Cleans up all changes related to the linking process
Watch mode - Skips linking and just starts watching. Requires previous link. Used if something really bad happens and the watchers give up, or you accidentally quit the watch mode.

The --debug flag will log all stdout, stderr and errors to the console in chalk.blue.

Linking

Linking requires you to bootstrap and start kibana before linking.

The only other prompt is for the relative path from @elastic/charts directory to kibana directory.

The script does all of the following:

  • Builds @elastic/charts is watch mode
  • Replaces all linkedPackages require statements with relative paths (require("react") becomes require("../path/to/kibana/react"))
  • Builds @kbn/ui-shared-deps in watch mode

The struggle 😫

The @kbn/ui-shared-deps package is used, to my best understanding, as a poor-mans yarn resolution creating a single export for singlton packages and packages needing consistent versions. Most important is react as hooks require the use of the same react instance. Linking charts to kibana would enable kibana to use the linked react but that local charts would use the local react (i.e. elastic-charts/node_modules_react).

If we then attempt to link react in charts to point at react in kibana, it would use the correct react (i.e. path/to/kibana/node_modules/react BUT the way that webpack bundles @kbn/ui-shared-deps this will not be the same instance of react and thus still throwing the dreaded invalid hook call warning so symlinking react does not work.

The solution is to replace all the paths in the built chart dist output to thier relative path to the kibana react. This will use the webpack exported instance of react from @kbn/ui-shared-deps. This however is not going to work just replacing the react since other packages like react-redux point to react internally and since that is the react-redux local to charts it will be wrong yet again. So the solution is to replace react, react-dom, react-redux and redux. So every change that happens in charts needs to be built, swap the links and build @kbn/ui-shared-deps to be reflected in kibana.

The good news is that there are no changes to src in either kibana nor charts for linking to take place. This makes it easy to cleanup the linked files once no longer needed.

But how do I know it's linked and what charts directory is linked?

I added a console.log to clearly indicate when charts is linked and was chart directory is linked.

image

Limitations

  • You can only link one charts directory to one kibana directory
  • Not all errors are handled though many are passed through command line interface to resolve.
  • Many get in a stuck state as it's not tested in the wild...yet!
  • Only rebuilds .ts files, not scss
  • Cannot run nvm install to update kibana on the fly, requires user to update node manually.

packages/link_kibana/process_utils.js Show resolved Hide resolved
packages/link_kibana/utils.js Outdated Show resolved Hide resolved
@nickofthyme nickofthyme added the :build Build tools / dependencies label May 24, 2021
@markov00 markov00 mentioned this pull request May 26, 2021
3 tasks
@nickofthyme nickofthyme requested a review from markov00 May 26, 2021 16:34
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested locally and I love how fast we can now test the changes to the library directly within Kibana!
Thanks Nick, that's awesome!!!

if (action === 'Link') {
const { appLinkRelativePath } = await inquirer.prompt({
name: 'appLinkRelativePath',
message: 'Enter path to applitcation directory to link',
Copy link
Member

Choose a reason for hiding this comment

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

typo

@nickofthyme nickofthyme enabled auto-merge (squash) May 27, 2021 22:33
@nickofthyme nickofthyme merged commit 5b26dbe into elastic:master May 27, 2021
@nickofthyme nickofthyme deleted the link-script branch May 28, 2021 02:05
@nickofthyme
Copy link
Collaborator Author

🎉 This PR is included in version 30.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:build Build tools / dependencies released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants