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.

Sign up for GitHub

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

fix(appconfig): Fix the default value for inlining CSS #42

Merged
merged 1 commit into from
Oct 19, 2023
Merged

fix(appconfig): Fix the default value for inlining CSS #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions __tests__/appconfig.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@
*
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

// ok as this is just for tests
// eslint-disable-next-line n/no-extraneous-import
import type { RollupOutput, OutputOptions, OutputChunk } from 'rollup'
import { build, resolveConfig } from 'vite'
import { describe, it, expect } from 'vitest'
import { createAppConfig } from '../lib/appConfig'
import { AppOptions, createAppConfig } from '../lib/appConfig'
import { fileURLToPath } from 'url'
import { resolve } from 'path'
import { LibraryOptions } from '../lib/libConfig'
import cssInjectedByJsPlugin from 'vite-plugin-css-injected-by-js'

const __dirname = fileURLToPath(new URL('.', import.meta.url))

Expand Down Expand Up @@ -65,7 +66,31 @@ describe('app config', () => {
expect(assetFileNames({ name: 'foo.css' })).toBe('css/@nextcloud-vite-config-[name].css')
})

const createConfig = async (command: 'build' | 'serve' = 'build', mode: 'development' | 'production' = 'production', options?: LibraryOptions) => await resolveConfig(await createAppConfig({
describe('inlining css', () => {
const pluginName = cssInjectedByJsPlugin().name

it('does not inline css by default', async () => {
const resolved = await createConfig()
expect(resolved.plugins.filter(({ name }) => name === pluginName)).toHaveLength(0)
})

it('does not inline css when disabled', async () => {
const resolved = await createConfig('build', 'production', { inlineCSS: false })
expect(resolved.plugins.filter(({ name }) => name === pluginName)).toHaveLength(0)
})

it('does inline css when enabled', async () => {
const resolved = await createConfig('build', 'production', { inlineCSS: true })
expect(resolved.plugins.filter(({ name }) => name === pluginName)).toHaveLength(1)
})

it('does inline css when enabled with configuration', async () => {
const resolved = await createConfig('build', 'production', { inlineCSS: { useStrictCSP: true } })
expect(resolved.plugins.filter(({ name }) => name === pluginName)).toHaveLength(1)
})
})

const createConfig = async (command: 'build' | 'serve' = 'build', mode: 'development' | 'production' = 'production', options?: AppOptions) => await resolveConfig(await createAppConfig({
main: 'src/main.js',
}, options)({ command, mode, ssrBuild: false }), command)
})
15 changes: 11 additions & 4 deletions lib/appConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import type { UserConfig, UserConfigFn } from 'vite'
import type { PluginConfiguration as VitePluginInjectCSSOptions } from 'vite-plugin-css-injected-by-js/dist/esm/declarations/interface.js'
import type { BaseOptions, NodePolyfillsOptions } from './baseConfig.js'

import { mergeConfig } from 'vite'
Expand All @@ -18,7 +19,13 @@ export const appName = process.env.npm_package_name
export const appVersion = process.env.npm_package_version
export const appNameSanitized = appName.replace(/[/\\]/, '-')

export interface AppOptions extends BaseOptions {
export interface AppOptions extends Omit<BaseOptions, 'inlineCSS'> {
/**
* Inject all styles inside the javascript bundle instead of emitting a .css file
* @default false
*/
inlineCSS?: boolean | VitePluginInjectCSSOptions,
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case there we would like to inline CSS in an App, not a lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is currently the default for webpack built apps, so probably yes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed 🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sad face? This is quite ok, no?
Saves us to manually add the stylesheet with addStyle as well as a few bytes of additional requests (and reduce the loading queue)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sad face? This is quite ok, no?

It is worse in a page performance. One more request doesn't change loading as much (if it is not HTTP/1, it is likely a parallel request) as loading more JS (not in parallel), parsing JS code and executing it. Which doesn't replace parsing and applying css, it is an additional expensive blocking task. So an app launches later.

CSS stored in JS also has more size. 50kb css ~= 200kb css in a js bundle on a small experiment with Talk.

And adding styles in runtime increases the number of layout operations in the rendering pipeline, making the initial rendering slower (there are no app styles from the initial load), but it is unlikely noticeable on our load with a super large JS.

style-loader is usually used for development because it is faster on bundling and HMR (and on small libs).

Copy link
Contributor

Choose a reason for hiding this comment

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

How would you do that for libs then?
That means we'd have to import the css manually all the time too?

Copy link
Contributor

@ShGKme ShGKme Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How would you do that for libs then? That means we'd have to import the css manually all the time too?

In general — yes. A common practice for libs is to build and import css completely separately. Because this is both performant and "close to native" approach independent from bundler's config or a test runner.

For example, vue-virtual-scroller or vue-select are imported as

import { VueSelect } from 'vue-select'
import 'vue-select/dist/vue-select.css'

Or our @nextcloud/password-confirmation

import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/style.css' // Required for dialog styles

This is not great for libs with a dozen exports like @nextcloud/vue if we want to have a tree-shaking or per-component import. In this case, it can be handled by bundlers.

Currently @nextcloud/vue is already building separated js/css assets. Then it requires/imports .css files from .[mc]js modules. This is not correct for nodejs/browser, but it is handled by bundlers.

// @nextcloud/vue/dist/Components/NcButton.mjs
import "../assets/index-4a775ba1.css";
import { n as f } from "../chunks/_plugin-vue_normalizer-71e2aa87.mjs";

All CSS assets from @nextcloud/vue:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could disable injecting the CSS, vite can handle multiple entry points and will produce one CSS file for each entry point.
So if your app provides lets say main.js and settings.js, you will also get main.css and settings.css.

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 disable injecting the CSS, vite can handle multiple entry points and will produce one CSS file for each entry point. So if your app provides lets say main.js and settings.js, you will also get main.css and settings.css.

Also works with webpack config with a simple adjustment (adding MiniCssExtractPlugin)


/**
* Whether to empty the output directory (`js/`)
* @default true
Expand Down Expand Up @@ -66,7 +73,7 @@ export const createAppConfig = (entries: { [entryAlias: string]: string }, optio
}

return createBaseConfig({
...options,
...(options as BaseOptions),
config: async (env) => {
console.info(`Building ${appName} for ${env.mode}`)

Expand All @@ -76,8 +83,8 @@ export const createAppConfig = (entries: { [entryAlias: string]: string }, optio

const plugins = []
// Inject all imported styles into the javascript bundle by creating dynamic styles on the document
if (options?.inlineCSS !== false) {
plugins.push(injectCSSPlugin())
if (options.inlineCSS) {
plugins.push(injectCSSPlugin(typeof options.inlineCSS === 'object' ? options.inlineCSS : undefined))
}

// defaults to true so only not adding if explicitly set to false
Expand Down
Loading