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

Update infrastructure to avoid "fat" Candela bundle #503

Merged
merged 105 commits into from
Jul 7, 2017
Merged

Conversation

waxlamp
Copy link
Member

@waxlamp waxlamp commented Jun 13, 2017

UPDATE: further changes were made on this PR superseding the description below the break.

This PR introduces three ways to incorporate Candela into a client project:

  1. Easy mode. Include one of the built bundles - candela-all.js or candela-all.min.js in a <script> tag or a webpack project. These bundles contain everything available in the codebase, bundled together with all dependencies, forming a large but ready-to-go bundle.

  2. Medium mode. Candela now partitions the built-in components into "plugins"; each plugin has an index.js file exporting the components, and a load.js file that automatically adds those components to candela.components. This method requires using Webpack for the client project, and results in reasonable bundle sizes, only including whatever the plugins you load require. Since the other components come more or less for free once the dependencies for a given plugin are included, this caps the maximum size of your resulting application bundle.

  3. Beast mode. You can forgo the plugin loaders by importing or require()ing just the exact files/components you need for your project, again using Webpack. This approach will theoretically give you the smallest bundle size for the portions of Candela you wish to use.

TODO:

  • Update documentation
  • Add sourcemaps to the production build
  • Verify .npmignore specs

(the following is outdated)

Previously, we were building Candela like an application bundle, which in particular meant bundling all dependencies into the resulting code bundle.

The main change here is to use the externals option of Webpack to avoid doing so. Instead, it will now be incumbent on the client to include the correct libraries when using Candela. The final bundle will still be relatively large, but won't include anything that's not being used, and won't include multiple copies of a single library, etc.

This PR also builds multiple plugin "sub-bundles" to go along with candela.js and candela.min.js. Clients can include these directly, or they can reference the source files and use Webpack to build the final application bundle, etc.

TODO:

  • update documentation
  • fold information in peerDependencies fields of plugins' package.json files into documentation
  • reword commit messages with feat as appropriate
  • update .npmignore
  • update CONTRIBUTING.md and build docs

@codecov-io
Copy link

codecov-io commented Jun 13, 2017

Codecov Report

Merging #503 into master will decrease coverage by 0.71%.
The diff coverage is 95.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   48.43%   47.72%   -0.72%     
==========================================
  Files          40       62      +22     
  Lines        1627     1645      +18     
==========================================
- Hits          788      785       -3     
- Misses        839      860      +21
Impacted Files Coverage Δ
plugins/mixin/Events.js 100% <ø> (ø)
plugins/trackerdash/TrackerDash/colors.js 100% <ø> (ø)
plugins/vega/index.js 100% <ø> (ø)
plugins/trackerdash/TrackerDash/utility.js 26.66% <ø> (ø)
plugins/mixin/Resize.js 50% <ø> (ø)
plugins/mixin/InitSize.js 33.33% <ø> (ø)
plugins/trackerdash/TrackerDash/styles/index.js 100% <ø> (ø)
plugins/mixin/AutoResize.js 37.5% <ø> (ø)
plugins/mixin/VegaChart.js 83.87% <ø> (ø)
plugins/trackerdash/TrackerDash/ResultTablePane.js 9.45% <100%> (ø)
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c8a1b2...237f28b. Read the comment docs.

@waxlamp waxlamp changed the title [WIP] Update infrastructure to avoid "fat" Candela bundle Update infrastructure to avoid "fat" Candela bundle Jun 13, 2017
@waxlamp waxlamp requested a review from jeffbaumes June 13, 2017 23:56
@waxlamp
Copy link
Member Author

waxlamp commented Jun 13, 2017

@jeffbaumes PTAL.

I realize there are some major changes in this PR, so if you'd like to schedule a time to look this over together, please let me know.

waxlamp added 25 commits June 19, 2017 10:31
Any components wishing to use d3 v4 must make use of the d3
microlibraries instead.
Now, the microlibraries will be used to popular candela.components
one-by-one as needed by a particular client application.
This commit also removes unneeded dependencies from the top-level
package.json file.
@jeffbaumes
Copy link
Member

I was able to get the candela-all bundle working in a script tag, but there is strangeness in calling it candela-all. Since it is not a valid JS name, I could only get it by using an accessor off of the window object. Perhaps a reason to just call the bundle candela?

This template works if you place it as index.html in the root of the Candela repo and serve the directory.

<body>
<script src="dist/candela-all.js" charset="utf-8"></script>
<script>
  var candela = window['candela-all'];
  console.log(candela);
  var el = document.createElement('div')
  document.body.appendChild(el);

  var data = [];
  for (var d = 0; d < 10; d += 1) {
    data.push({
      a: d,
      b: d
    });
  }

  var vis = new candela.components.BarChart(el, {
    data: data,
    x: 'a',
    y: 'b'
  });
  vis.render();
</script>
</body>

@waxlamp
Copy link
Member Author

waxlamp commented Jul 3, 2017

I think calling it candela.js is fine, I'll make the appropriate change.

It is really cool that we can just drop an HTML file in the repo root, build the project, and have a working "app" just by serving that file. Do you think we should include an example like that in the repo? Maybe as a one-off in the examples directory?

@jeffbaumes
Copy link
Member

Sure I like that idea - would be great if it worked with the run:examples npm task but I don't think that one currently serves high enough in the source tree to be able to refer to the dist directory, am I correct? Having a second run:serve or something similar might be convenient (or change the examples one to serve the root).

@waxlamp
Copy link
Member Author

waxlamp commented Jul 3, 2017

@jeffbaumes, take a look at 237f28b (changes candela-all to candela in the dist directory).

I'll open an issue about the HTML example - I think I'd rather just get this PR merged at this point.

Copy link
Member

@jeffbaumes jeffbaumes left a comment

Choose a reason for hiding this comment

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

npm run examples does not work - seems that index.html needs to reference examples.js instead of ../examples/examples.js. Also some of the linked examples (treeheatmap for example?) don't seem to work - it reports a missing .js file.

Otherwise this looks good.

Copy link
Member

@jeffbaumes jeffbaumes left a comment

Choose a reason for hiding this comment

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

I've confirmed that the examples problem was on my end (old build artifacts). There may be lingering doc issues, and we will need to monitor the girder candela plugin build if it tries to update to the version this merge creates, but LGTM

@waxlamp
Copy link
Member Author

waxlamp commented Jul 7, 2017

@jeffbaumes thanks for the (unusually painful) review!

@waxlamp waxlamp merged commit 9a3c04e into master Jul 7, 2017
@waxlamp waxlamp deleted the slim-bundle branch July 7, 2017 18:28
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.

3 participants