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

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

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Oct 11, 2023

The docs said false but it was true

@susnux susnux added bug Something isn't working 3. to review Ready to review labels Oct 11, 2023
@susnux susnux requested a review from ShGKme October 11, 2023 23:55
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #42 (9bd2641) into main (b1e4978) will decrease coverage by 0.13%.
Report is 1 commits behind head on main.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   81.81%   81.69%   -0.13%     
==========================================
  Files           8        8              
  Lines         572      579       +7     
  Branches       46       48       +2     
==========================================
+ Hits          468      473       +5     
  Misses         58       58              
- Partials       46       48       +2     
Files Coverage Δ
lib/appConfig.ts 79.64% <81.81%> (-0.36%) ⬇️

lib/appConfig.ts Outdated Show resolved Hide resolved
Comment on lines +23 to +27
/**
* Inject all styles inside the javascript bundle instead of emitting a .css file
* @default false
*/
inlineCSS?: boolean | VitePluginInjectCSSOptions,
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

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)

The docs said `false` but it was `true`

Co-authored-by: Grigorii K. Shartsev <[email protected]>
Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux merged commit 5af9ca6 into main Oct 19, 2023
10 of 11 checks passed
@susnux susnux deleted the fix/default-value branch October 19, 2023 10:21
@susnux susnux mentioned this pull request Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants