Skip to content
This repository has been archived by the owner on Sep 25, 2019. It is now read-only.

Add configuration for vendor and commons chunk #417

Merged
merged 11 commits into from
Jan 2, 2018

Conversation

Deschtex
Copy link
Member

Closes #415

Copy link
Member

@pirelenito pirelenito left a comment

Choose a reason for hiding this comment

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

Awesome work!

Besides the comments I've add, it would be nice to have an integration test as well for vendor and common.

Wdyt?

README.md Outdated
```js
// sagui.config.js
module.exports = {
pages: ['index', { name: 'about', independent: true }]
Copy link
Member

Choose a reason for hiding this comment

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

Very small suggestion, but what about using a demo or playground in the name, to better hint this could be used for development pages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Will fix it.

README.md Outdated

#### `vendor`

If you want all your external dependencies in your [pages](#pages) to be bundled together in a "vendor" chunk, then set this flag to `true`. By default it is set to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

...external dependencies (node_modules)...

@@ -19,7 +19,7 @@ import buildLoadersConfig from './loaders'
* @return ready to use webpack configuration as an array
*/
export default (saguiConfig = {}) => {
const { pages = [], libraries = [], ...sharedSaguiConfig } = saguiConfig
const { pages = [], libraries = [], chunks = {}, ...sharedSaguiConfig } = saguiConfig
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need to extract chunks here. Just let it in the sharedSaguiConfig.

Then, you can remove the extra parameter you've add (chunks) in a couple of places.

Makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix it

const common = plugins[1]

expect(vendor.selectedChunks[0]).equal('index')
expect(vendor.selectedChunks[2]).equal(undefined)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe test the length of the selectedChunks here to be equal 1?

Copy link
Member Author

@Deschtex Deschtex Dec 26, 2017

Choose a reason for hiding this comment

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

For some reason there's an undefined on every other position in the array, so it will actually be 2. Will check that.

@Deschtex
Copy link
Member Author

@pirelenito I'd appreciate your help with the integration tests.

@pirelenito pirelenito force-pushed the add-configuration-for-vendor-and-commons-chunk branch from 79ad537 to 5884302 Compare January 2, 2018 15:42
Copy link
Member

@pirelenito pirelenito 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 updated the PR with integration tests! Should be ready to merge and release!

Thanks a lot @Deschtex ❤️

@pirelenito pirelenito merged commit 209e913 into master Jan 2, 2018
@pirelenito pirelenito deleted the add-configuration-for-vendor-and-commons-chunk branch January 2, 2018 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants