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

Implement code splitting #39

Merged
merged 1 commit into from
Nov 16, 2018
Merged

Implement code splitting #39

merged 1 commit into from
Nov 16, 2018

Conversation

ifeanyiisitor
Copy link
Contributor

This PR introduces anvil-plugin-code-splitting, which is meant to handle everything related to bundle / code splitting, including dynamic imports. The bundle splitting strategy used is the one outlined here https://hackernoon.com/the-100-correct-way-to-split-your-chunks-with-webpack-f8a9df5b7758

## Installation

```
npm install --save-dev @financial-times/anvil-plugin-code-splitting @financial-times/anvil-plugin-babel
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter which order you install these plugins in? It suggests further down that you need anvil-plugin-babel first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where npm install is concerned, it doesn't matter. The order only matters in anvil.config.json


## Usage

anvil.config.json
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably be clearer, we probably want something along the lines of Include an anvil.config.json file at the root of your app and list the dependent plugins. And maybe a short explanation of how this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'm intentionally keeping it short because Matt has a task to create a format for plugin docs

@@ -0,0 +1,12 @@
/**
* This file is meant to export the babel preset so that it can be used in a .babelrc file as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

You could get rid of 'is meant to` and just have 'This file exports...'

function amendWebpackConfig(runningContext: RunningWebpackContext) {
const baseConfig = runningContext.webpackConfig
const config = {
optimization: {
Copy link
Contributor

Choose a reason for hiding this comment

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

pedantic - but if it doesn't affect anything downstream we could prefer the British spelling of optimisation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not something that we have control over. It's a property of the webpack config.

@ifeanyiisitor ifeanyiisitor force-pushed the bundle-splitting branch 2 times, most recently from 4f60e0b to f5074f8 Compare November 16, 2018 16:11
@ifeanyiisitor ifeanyiisitor merged commit eedc8e9 into master Nov 16, 2018
@ifeanyiisitor ifeanyiisitor deleted the bundle-splitting branch November 16, 2018 16:54
minSize: 0,
cacheGroups: {
vendor: {
test: /[\\/]node_modules[\\/]/,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to expose the cacheGroups property via Adonai or make this otherwise configurable in an FT-specific way.

I think the FT "vendor" chunk will be commonly used modules rather than any 3rd party code so that the bundle may be shared between multiple applications.

I guess we'll need to sit down and plan this all out.

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