nuxt-modules / apollo

Nuxt.js module to use Vue-Apollo. The Apollo integration for GraphQL.
https://apollo.nuxtjs.org
MIT License
952 stars 198 forks source link

Apollo module brakes compilation of Element UI library when custom scss variables are used #141

Closed AndrewBogdanovTSS closed 6 years ago

AndrewBogdanovTSS commented 6 years ago

Version

v4.0.0-rc.1

Reproduction link

https://github.com/AndrewBogdanovTSS/nuxt-apollo-element-ui-test

Steps to reproduce

What is expected ?

Element UI library is building from its source code due to Nuxt 2 new transpile property that exists in nuxt config

What is actually happening?

× error F:\Projects\Study\Nuxt\nuxt-apollo-element-ui-test\node_modules\element-ui\lib\theme-chalk\fonts\element-icons.woff:1
  (function (exports, require, module, __filename, __dirname) { wOFF

  SyntaxError: Invalid or unexpected token
  at createScript (vm.js:80:10)
  at Object.runInThisContext (vm.js:139:10)
  at Module._compile (module.js:616:28)
  at Object.Module._extensions..js (module.js:663:10)
  at Module.load (module.js:565:32)
  at tryModuleLoad (module.js:505:12)
  at Function.Module._load (module.js:497:3)
  at Module.require (module.js:596:17)
  at require (internal/module.js:11:18)
  at r (F:\Projects\Study\Nuxt\nuxt-apollo-element-ui-test\node_modules\vue-server-renderer\build.js:8341:16)
  at Object.element-ui/lib/theme-chalk/fonts/element-icons.woff (server-bundle.js:3165:18)
  at __webpack_require__ (webpack/bootstrap:25:0)
  at Object../node_modules/css-loader/index.js?!./node_modules/postcss-loader/lib/index.js?!./node_modules/sass-loader/lib/loader.js?!./assets/scss/
index.scss (assets/scss/index.scss:7:0)
  at __webpack_require__ (webpack/bootstrap:25:0)
  at Object../assets/scss/index.scss (assets/scss/index.scss?0905:4:0)
  at __webpack_require__ (webpack/bootstrap:25:0)

Additional comments?

This error is happening only once apollo module is connected to the project. It seems like nuxt/apollo interfere with the build process in is some way that prevents it from working properly. cc: @dohomi

This bug report is available on Nuxt community (#c126)
kavalcante commented 6 years ago

I had the same error, solves when adding element-ui/lib/theme-chalk/display.css in css array on nuxt.config.js

kavalcante commented 6 years ago

Another aproach is doing this at element-ui.js:

import '../node_modules/element-ui/lib/theme-chalk/index.css';
dohomi commented 6 years ago

as I con't use element-ui I'm not sure how to fix it. any PR is welcome

AndrewBogdanovTSS commented 6 years ago

@kavalcante suggested workaround doesn't work for me, but even if it did, I think such workarounds actually brakes the whole purpose of partial Element UI compilation since you have to include all CSS without an ability to modify stuff over SCSS and reduce the bundle size. @dohomi same is happening with Bootstrap-Vue library: https://cmty.app/nuxt/apollo-module/issues/c125 I wonder how is it possible for apollo module to alter unrelated configs? Does it mutate webpack configs in some way?

kavalcante commented 6 years ago

@AndrewBogdanovTSS Actually you can import partial stuffs and use SCSS to reduce bundle size, with this:

import '../theme/index.css';

But I do agree that is just a workaround. Maybe is useful for someone until problem be solved.

AndrewBogdanovTSS commented 6 years ago
import '../theme/index.css';

@kavalcante wouldn't it import all css of the theme completely?

kavalcante commented 6 years ago

It's just a example, using the custom theme generator.

The problem is happening when add apollo-module it's that any relative import is not been properly imported by webpack. Don't know why. For that reason, when you use for example:

import Modal from 'bootstrap-vue/es/components/modal';

It's not working, unless you use:

import Modal from '../node_modules/bootstrap-vue/es/components/modal';

Of course, this is just a workaround for who can't wait or don't know yet how to fix this error, like me ;)

kavalcante commented 6 years ago

I try to debug the problem, the error is happening when using this way:

import 'LIB/relative-path'

And in my reproduction it's happen only with nuxt, and not apollo-module. Maybe this issue and #140 need be moved to nuxt official repository. What do you think @dohomi?

Again, to solve this issue, you can do like this:

import '../node_modules/LIB/relative-path';

Reproduction: https://github.com/kavalcante/nuxt-import-error

dohomi commented 6 years ago

In my opinion the problem described here is how to import other component libraries. I don't have experience in element-ui or other mentioned libs (I use vuetify). I needed to enable es5 rules inside of my webpack config to import components. This module is most likely not responsible for the thrown errors but the NuxtJs webpack config

AndrewBogdanovTSS commented 6 years ago

I needed to enable es5 rules inside of my webpack config to import components.

@dohomi and that's exactly what's causing the issue. Right now you current config is written in such a way that it overwrites anything that was previously whitelisted. Instead, it should augment current list of libs that already whitelisted. I was able to fix the problem by changing node_modules/@nuxtjs/apollo/lib/module.js:64 line from:

whitelist: [/^vue-cli-plugin-apollo/]

to

whitelist: [/^vue-cli-plugin-apollo/, /^element-ui/]

Another way to handle it would be fully comment the use of nodeExternals in the apollo module and handle it manually in nuxt.config by adding:

build: {
        extend (config, { isServer }) {
            if (isServer) {
                config.externals = [
                    nodeExternals({
                        whitelist: [/^vue-cli-plugin-apollo/, /^element-ui/]
                    })
                ]
            }
        }
    }

maybe some config parameter should be added to nuxt/apollo to allow manual whitelisting as a quick fix? I guess the same will be relevant to a bt-vue issue since new 'transpile' property also uses nodeExternals under the hood. @Atinux maybe you could suggest on how such issues can be handled from nuxt.config.js ? Maybe transpile property should dominate over libs internal configs?

dohomi commented 6 years ago

@AndrewBogdanovTSS I thought that is chained and not overwritten. @Atinux @Akryum do you guys know the right way of setting nodeExternals? I am not sure what would be the right setup.

atinux commented 6 years ago

@dohomi You can use build.transpile with Nuxt.js, see https://github.com/Atinux/nuxt-bt-test/blob/master/nuxt.config.js#L4

dohomi commented 6 years ago

@Atinux thanks for the heads up. could somebody with this issue open a PR and see if the use of transpile works to fix the bug with bootstrap/element-ui? I don't have the right setup at the moment and not a lot of time - work is currently eating me alive

AndrewBogdanovTSS commented 6 years ago

@Atinux @dohomi I can confirm that it's working when transpile option is used. I replaced

if (isServer) {
      config.externals = [
        nodeExternals({
          whitelist: [/^vue-cli-plugin-apollo/]
        })
      ]
    }

with

this.options.build.transpile.push(/^vue-cli-plugin-apollo/)

and it started working properly, at least with element-ui lib (I will try it with bt-vue a bit later). The bonus point in all of this is that specifying additional transpile in the project config for element-ui wasn't even necessary. Though I have some concerns with the behavior of transpile option. @Atinux if I'll specify additional transpile options in projects config will it overwrite anything that was specified in 3rd party modules or augment to it?

AndrewBogdanovTSS commented 6 years ago

Ok, so I can confirm that the same approach is working flawlessly with bt-vue, now I can specify

build: {
        transpile: ['bootstrap-vue']
    },

and it won't be overwritten by Apollo! Hurrray! @dohomi when will we be able to push this update to apollo module? I really need this. Speaking about some additional improvements, using addVendor is no longer necessary with Webpack 4 and can be removed.

// Add vue-apollo and apollo-client in common bundle
  this.addVendor(['vue-apollo', 'js-cookie', 'cookie'])

It looks like we really need an updated version of @nuxtjs/apollo that would be fully compatible with Nuxt 2

dohomi commented 6 years ago

@AndrewBogdanovTSS this package is still using Nuxt 1.4.x and not nuxt-edge so I can't remove the vendor (yet). I still wait for the v2 release to add this change.

@AndrewBogdanovTSS would you mind open a PR with your changes? I think this change may affect other open issues as well.

AndrewBogdanovTSS commented 6 years ago

@dohomi I sure can. My only concern - should we keep consistency with Nuxt 1? @Atinux maybe you could recommend a solution that would work for Nuxt 1.4 since transpile option won't be available for this version of Nuxt ?

AndrewBogdanovTSS commented 6 years ago

@dohomi how can I make a PR into your repo? When I try creating a branch and push my changes I get a 403 forbidden error. Should I for it first?

dohomi commented 6 years ago

Can you guys try this PR and see if it fixes the issue: https://github.com/nuxt-community/apollo-module/pull/158

AndrewBogdanovTSS commented 6 years ago

@dohomi when will this fix be released? I suppose it should be released under rc2, but when will it be released?

dohomi commented 6 years ago

did you try the PR? I currently face an issue which breaks my environment so I can't release it atm...

AndrewBogdanovTSS commented 6 years ago

@dohomi yes I've tested it before committing and it works well for me. What exact issues are you facing with it?

dohomi commented 6 years ago

@AndrewBogdanovTSS please read the comments of #158 I face an issue with the current beta.25 at the moment thats why rc2 isn't released yet

negezor commented 6 years ago

Now sometimes can not find Vue.extend()

dohomi commented 6 years ago

rc2 just released but this won't fix this issue. Still waiting until #158 can be merged

dohomi commented 6 years ago

rc3 got released. please check if the issue is resolved for now

dohomi commented 6 years ago

I think with either 4.0.0.rc2.2 or rc3 this issue is resolved. But rc3 does not work with async components yet. Will close this now, feel free to reopen if the issue persist

AndrewBogdanovTSS commented 6 years ago

I checked it with 4.0.0.rc2.2 and it indeed worked. Thanks a lot for your help, @dohomi 👍