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

Move to shared babel/webpack config #1659

Merged
merged 9 commits into from
Jul 14, 2021
Merged

Move to shared babel/webpack config #1659

merged 9 commits into from
Jul 14, 2021

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jun 16, 2021

  • eslint
  • babel
    • /home/runner/work/text/text/src/components/PublicFilesEditor.vue Error: 71:4 error Unexpected mutation of "active" prop vue/no-mutating-props
  • stylelint
  • webpack
    • Syntax highlight chunks are not properly generated yet
    • Entrypoints changed so we need to update the template imports

webpack.js Show resolved Hide resolved
Comment on lines +15 to +23
webpackConfig.optimization.splitChunks.cacheGroups = {
defaultVendors: {
test(module) {
return module.resource && module.resource.includes(`${path.sep}node_modules${path.sep}`) &&
!module.resource.includes(`${path.sep}highlight.js${path.sep}`)
},
name: 'vendors',
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

it makes sure that vendor files get into a separate chunk, except the highlight js ones which should stay at one chunk per supported syntax highlighting language.

Copy link
Member

Choose a reason for hiding this comment

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

What benefit do we get from this?

Copy link
Member

Choose a reason for hiding this comment

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

Vendors are nice to split as long as they don't get all used at the same time, right?
Like dynamic loading?
But if we always end up loading 90% of the chunks, we just create more bandwith and doesn't help cleaning the bundles imho :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue i have is that I fail to tell webpack to only split off the highlight.js chunks and the ones that are dynamically imported (e.g. for only loading the files app init code and not the whole viewer component right away). Therefore I went for that hybrid approach that unfortunately leaves one extra chunk.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to look into that into a follow up again 😉

Copy link
Member

Choose a reason for hiding this comment

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

You can do that with an ignore plugin and do a dynamic import of the highlight lib :)
https://github.com/nextcloud/contacts/blob/017026a8e42717cc8e1935af5ce61aa46b3e9f79/webpack.js#L8

Copy link
Member

Choose a reason for hiding this comment

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

Iirc, any direct runtime import will overrule compile time bundling

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit d1ca93f into master Jul 14, 2021
@juliusknorr juliusknorr deleted the enh/node7 branch July 14, 2021 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants