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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for SSR style injection #126

Merged
merged 1 commit into from Jul 15, 2020

Conversation

adrienbaron
Copy link
Contributor

@adrienbaron adrienbaron commented Apr 27, 2020

This PR adds support to inject Vuetify styles on SSR.

This fixes issues of styles not being included in the SSR response when using Nuxt + Vuetify:

This feature is based on an idea from @freddy38510. It works as follows:

  • We track all the .sass files that are imported by each Vuetify group (VTabs, VCard...).
  • We store in a Map the mapping between each Vuetify component and the Set of .sass files their group depends on.
  • When vuetify-loader adds the imports to Vuetify in a component, it also adds a conditional block that registers a beforeCreate hook only on SSR (when process.server is true).
  • When that beforeCreate hook is called, it require each .sass file and calls __inject__ from vue-style-loader on them.

A new option registerStylesSSR (set by default to false) has been added to enable the behaviour.
When it's set to true, you need to make sure to tell vue-style-loader to enable manualInject.

I've tested this with a relatively big project (https://www.clashofstats.com/) and it seems to work as expected 馃憤.

Feedback on this PR is very welcome!

@johnleider johnleider requested a review from KaelWD April 28, 2020 14:20
@adrienbaron adrienbaron marked this pull request as draft April 28, 2020 16:05
@adrienbaron
Copy link
Contributor Author

Found an issue where this change seems to fail on GitHub CI, will investigate and update when fixed

@adrienbaron adrienbaron marked this pull request as ready for review April 28, 2020 17:11
@adrienbaron
Copy link
Contributor Author

adrienbaron commented Apr 28, 2020

Ok, I was using parent.path in the Module._Loader function, turns out it's Node 12.x only. Updated to use parent.filename and it works in Node 10.x 馃憤

@KaelWD
Copy link
Member

KaelWD commented May 1, 2020

This applies to anyone using vue-style-loader with manualInject right, not just nuxt/ssr? The nuxt-specific part should probably go in the @nuxtjs/vuetify documentation instead.

@adrienbaron
Copy link
Contributor Author

@KaelWD Yes it does 馃憤. I don't know if many people use Vue SSR + Vuetify without Nuxt? This fix would probably work for them too, but I haven't tested.
Will make the documentation more generic and open an MR on the @nuxtjs/vuetify module with the Nuxt specific part 馃憤

@adrienbaron
Copy link
Contributor Author

@KaelWD Should be good to go, updated the documentation to be agnostic and just mention Vuetify as an example 馃憤

@asennoussi
Copy link

@KaelWD Could you please check this out?
It would be super helpful not to load the global vuetify css, but instead load only the needed CSS. <3

@adrienbaron
Copy link
Contributor Author

adrienbaron commented May 16, 2020

@KaelWD Could you please check this out?

It would be super helpful not to load the global vuetify css, but instead load only the needed CSS. <3

@Sshuichi If you want to try it in the meantime, feel free to use my branch instead of the npm package in your package.json:

"vuetify-loader": "git+https://github.com/adrienbaron/vuetify-loader.git"

@asennoussi
Copy link

@adrienbaron will give it a shot now. Thank you!

@Dangelfidel
Copy link

Dangelfidel commented Jul 5, 2020

@adrienbaron I tried your "vuetify-loader" implementation. But I am still facing FOUC.

This is my nuxt.config.js

import colors from 'vuetify/es5/util/colors'
import webpack from 'webpack'
export default {
apps: [
{
name: 'NuxtAppName',
exec_mode: 'cluster',
instances: 'max', // Or a number of instances
script: './node_modules/nuxt/bin/nuxt.js',
args: 'start'
}
],
server: {
port: 8000, // default: 3000
host: '0.0.0.0' // default: localhost
},
mode: 'universal',
/*
** Headers of the page
/
head: {
titleTemplate: 'AlziNet',
title: process.env.npm_package_name || '',
meta: [
{ charset: 'utf-8' },
{ name: 'viewport', content: 'width=device-width, initial-scale=1' },
{
hid: 'description',
name: 'description',
content: process.env.npm_package_description || ''
}
],
link: [{ rel: 'icon', type: 'image/x-icon', href: '/favicon.ico' }]
},
/

** Customize the progress-bar color
/
loading: { color: '#fff' },
/

** Global CSS
/
css: ['@/assets/variables.scss'],
/

** Plugins to load before mounting the App
/
/

** You can add configuration to webpack
*
/
build: {
build: {
extractCSS: true,
optimization: {
splitChunks: {
cacheGroups: {
styles: {
name: 'styles',
test: /.(css|vue)$/,
chunks: 'all',
enforce: true
}
}
}
}
},
plugins: [
new webpack.ProvidePlugin({
// global modules
_: 'lodash'
})
],
analyze: false,
transpile: ['vee-validate/dist/rules'],
extend(config, ctx) {}
},
/

** Nuxt.js dev-modules
/
buildModules: [
'@nuxtjs/eslint-module',
'@nuxtjs/stylelint-module',
'@nuxtjs/vuetify',
'@nuxtjs/date-fns'
],
/

** Nuxt.js modules
/
modules: [
'@nuxtjs/axios',
'@nuxtjs/pwa',
'@nuxtjs/dotenv',
'@nuxtjs/proxy',
'@nuxtjs/auth'
],
/

** Axios module configuration
** See https://axios.nuxtjs.org/options
/
axios: {
baseURL:
},
plugins: ['~/plugins/vee-validate.js'],
/

** vuetify module configuration
** https://github.com/nuxt-community/vuetify-module
/
vuetify: {
customVariables: ['~/assets/variables.scss'],
theme: {
dark: false,
themes: {
dark: {
primary: '#34421E',
accent: colors.grey.darken3,
secondary: '#C19434',
info: colors.teal.lighten1,
warning: colors.amber.base,
error: '#9B111E',
success: '#808000',
third: '#F1F2EF'
},
light: {
primary: '#34421E',
accent: colors.grey.darken3,
secondary: '#C19434',
info: colors.teal.lighten1,
warning: colors.amber.base,
error: '#9B111E',
success: '#808000',
third: '#F1F2EF'
}
}
}
},
/

** Build configuration
/
/

** Add your global variables here
*/
env: {
baseUrl:
},
router: {
middleware: ['auth']
},
auth: {
strategies: {
local: {
endpoints: {
login: {
url: '/api/token',
method: 'post',
propertyName: 'token'
},
logout: { url: '/api/auth/logout', method: 'post' },
user: { url: '/api/auth/user/', method: 'get' }
},
tokenType: 'bearer',
autoFetchUser: false
}
},
redirect: {
login: '/login',
logout: '/login',
callback: '/login',
home: '/dailyRecord'
}
},
proxy: {
'/api': {
target:
pathRewrite: {
'^/api': '/'
}
}
}
}

@adrienbaron
Copy link
Contributor Author

adrienbaron commented Jul 5, 2020

Hey @Dangelfidel !

There seem to be quite a few errors invalid syntax problems in your config, for example axios baseUrl doesn't have a value: axios: {baseURL:},.

Can you try editing your post and wrapping the config in triple backticks? This prevents Github messing with your formatting, like so:

```.
Put your config here and remove the ., I added them so that Github would display the ```.
```.

@adrienbaron
Copy link
Contributor Author

@KaelWD I've rebased the branch, sorry took me a while to do that. Is there anything I can do to help get this merged?

@KaelWD
Copy link
Member

KaelWD commented Jul 6, 2020

I've just gotta get off my ass and look at it.

@Dangelfidel
Copy link

Hey @Dangelfidel !

There seem to be quite a few errors invalid syntax problems in your config, for example axios baseUrl doesn't have a value: axios: {baseURL:},.

Can you try editing your post and wrapping the config in triple backticks? This prevents Github messing with your formatting, like so:

```.
Put your config here and remove the ., I added them so that Github would display the ```.
```.

I was doing something else wrong, after removing node_modules and installing all the dependencies againit works. Many thanks for your great work @adrienbaron

@adrienbaron
Copy link
Contributor Author

@KaelWD Rebased on 1.6.0 馃憤

lib/loader.js Show resolved Hide resolved
lib/loader.js Outdated Show resolved Hide resolved
lib/loader.js Outdated Show resolved Hide resolved
@KaelWD
Copy link
Member

KaelWD commented Jul 15, 2020

btw it's easier to review changes with a linear history, otherwise I have to mess around with git to see what you've done since last time I looked at it. merges and a million commits are fine because it'll all be squashed in the end.

@adrienbaron
Copy link
Contributor Author

@KaelWD Ah sorry about that, good shout, didn't think we would squash 馃憤 , will stop rewriting history now ^^"

})

// This makes sure Vuetify main styles will be injected.
// Using VApp as it's must be present for Vuetify to work, and it must only be there once.
styles.get('VApp').add('src/styles/main.sass')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about the style ordering here, but we can always come back and fix it if it turns out to be broken.

Copy link
Member

@KaelWD KaelWD left a comment

Choose a reason for hiding this comment

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

Great job, thanks!

@KaelWD KaelWD merged commit dd38100 into vuetifyjs:master Jul 15, 2020
@asennoussi
Copy link

Fuck Yes!!

@adrienbaron
Copy link
Contributor Author

@KaelWD is there a planning for when we might see a release containing this MR :)? Any help needed getting the release out?

@VesterDe
Copy link

VesterDe commented Sep 5, 2020

Very much looking forward to this release.

@neilpoulin
Copy link

I am also eagerly awaiting this release! Nice work.

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.

None yet

6 participants