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

Kick @code-dot-org/johnny-five out of the code-studio-common bundle #24478

Merged
merged 3 commits into from Aug 28, 2018

Conversation

balderdash
Copy link
Contributor

We use johnny-five in the maker toolkit, so it should live in the applab.js bundle. Instead, it's taking up a bunch of space in code-studio-common:
image

It got there because the shared toolbox settings cog checks if maker toolkit is enabled, and in doing so requires all the maker toolkit code. This PR moves the maker.isEnabled/maker.isAvailable methods to the maker toolkit redux module (which is where that enabled/available information comes from anyway) to break that dependency and move johnny-five back to the applab.js bundle.

@balderdash
Copy link
Contributor Author

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Awesome, nice catch.

Are we doing anything to ensure johnny-five stays in the applab.js bundle? Or do we just need to check on this manually every once in a while?

@balderdash
Copy link
Contributor Author

I've started logging bundle sizes, and I'm hoping to hook up some kind of test/PR status/monitoring to that so we can notice if a bundle gets unexpectedly larger. I'm not sure if it would be worth adding automated checks for specific modules that should/shouldn't be in specific bundles, at least not until we notice something causing problems repeatedly.

@balderdash balderdash merged commit 56d345b into staging Aug 28, 2018
@balderdash balderdash deleted the uncommon-johnny-five branch September 18, 2018 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants