nystudio107 / craft

nystudio107 Craft 3 CMS scaffolding project
BSD Zero Clause License
291 stars 85 forks source link

`vite.config.js` behavior is different than vanilla scaffolding when importing Flickity or jQuery #64

Closed cavellblood closed 3 years ago

cavellblood commented 3 years ago

Describe the bug

Vite in the craft-vite branch of this repo does not behave the same way when importing Flickity (or perhaps other packages as well) as it does in a new scaffolding when creating a new Vite app. I understand that it should be have a little differently because it is set up to work with Craft CMS but I would expect the import statement to behave similarly as it would with a new Vite app.

To reproduce

Steps to reproduce the behaviour:

  1. Clone the vite-craft branch of this repo.
  2. Run the following command in the buildchain directory:

    npm i flickity
  3. Then edit /src/js/app.ts by adding these two lines:

    import Flickity from "flickity";
    console.log(Flickity);

    Now the app.ts file should look like this:

    import Flickity from "flickity";
    console.log(Flickity);
    import App from "@/vue/App.vue";
    import { createApp } from "vue";
    
    // Import our CSS
    import "@/css/app.pcss";
    
    // App main
    const main = async () => {
      // Create our vue instance
      const app = createApp(App);
      // Mount the app
      const root = app.mount("#component-container");
    
      return root;
    };
    
    // Execute async function
    main().then((root) => {});
  4. Save the file and check the reloaded file contents in the browsers inspector. It should look similar to this:

    import Flickity from "/node_modules/flickity/js/index.js";
    console.log(Flickity);
    import App from "/src/vue/App.vue";
    import { createApp } from "/node_modules/vue/dist/vue.runtime.esm-bundler.js";
    import "/src/css/app.pcss";
    const main = async () => {
     const app = createApp(App);
     const root = app.mount("#component-container");
     return root;
    };
    main().then((root) => {
    });
    
    //# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJ....dfQ==
  5. Notice that in the browsers console you'll get an error like the following:
    Uncaught SyntaxError: import not found: default         app.ts:6

Expected behaviour

I would expect that it would behave like a new Vite scaffolding with the following steps.

  1. Run the following commands:
    npm init vite@latest my-vanila-app --template vanilla
    cd my-vanilla-app
    npm i flickity
  2. Open up the project in your IDE and edit the main.js file by adding these two lines at the top (or really anywhere in it. It all has the same effect):

    import Flickity from 'flickity'
    console.log(Flickity)

    Now the main.js file should look like this:

    import Flickity from 'flickity'
    console.log(Flickity)
    import './style.css'
    
    document.querySelector('#app').innerHTML = `
     <h1>Hello Vite!</h1>
     <a href="https://vitejs.dev/guide/features.html" target="_blank">Documentation</a>
    `
  3. Then run npm run dev
  4. Check the output of the main.js file in the network tab of the browsers inspector. It should look similar to this:

    import '/style.css'
    import __vite__cjsImport1_flickity from "/node_modules/.vite/flickity.js?v=a7f2a681"; const Flickity = __vite__cjsImport1_flickity.__esModule ? __vite__cjsImport1_flickity.default : __vite__cjsImport1_flickity
    
    console.log(Flickity)
    
    document.querySelector('#app').innerHTML = `
     <h1>Hello Vite!</h1>
     <a href="https://vitejs.dev/guide/features.html" target="_blank">Documentation</a>
    `
  5. I would expect the output of the import statement to look similar to this but it doesn't even look like it's processing it in the Craft setup:

    import __vite__cjsImport1_flickity from "/node_modules/.vite/flickity.js?v=a7f2a681"; const Flickity = __vite__cjsImport1_flickity.__esModule ? __vite__cjsImport1_flickity.default : __vite__cjsImport1_flickity

    but instead it is just like this:

    import Flickity from "/node_modules/flickity/js/index.js";
khalwat commented 3 years ago

I'm not sure I understand what you're saying here? Vite is Vite... what exactly isn't working?

cavellblood commented 3 years ago

Good question. I accidentally posted this issue before I was ready to publish it. The part that isn't working is that it's not importing the Flickity script in the craft version but it will in a new Vite scaffolding. If you do a console.log(Flickity) with the Vite scaffolding it will show this:

Screen Shot 2021-08-19 at 2 04 07 PM
cavellblood commented 3 years ago

But with the craft setup it will show this:

Screen Shot 2021-08-19 at 2 04 59 PM
cavellblood commented 3 years ago

I think the critical part is that with the vanilla scaffolding setup it's actually processing the file as seen here:

 import __vite__cjsImport1_flickity from "/node_modules/.vite/flickity.js?v=a7f2a681"; const Flickity = __vite__cjsImport1_flickity.__esModule ? __vite__cjsImport1_flickity.default : __vite__cjsImport1_flickity
khalwat commented 3 years ago

This shouldn't have anything to do with the Craft Vite plugin but rather whatever your Vite config is?

Try comparing the two vite.config.js from both projects.

cavellblood commented 3 years ago

Right. I've tried removing as much as I can from the craft project so that it will still work and it doesn't. The vanilla project doesn't even have a vite.config.js file.

cavellblood commented 3 years ago

I was wondering if it had something to do with it needing to interact with the back end. Perhaps this is more of a vitejs issue/question than a question for your craft setup.

khalwat commented 3 years ago

Can't imagine how that could ever be the case, Vite isn't even aware of the backend.

It looks to me like that in one case it's trying to do a CJS import of an old library, and in another case, it's trying to do an ESM import, and thus not finding default

cavellblood commented 3 years ago

Well I think it is aware of the backend with the Craft setup: https://vitejs.dev/guide/backend-integration.html

build: {
    emptyOutDir: true,
    manifest: true,
    outDir: '../cms/web/dist',
    rollupOptions: {
      input: {
        main: '/src/main.js',
      },
      output: {
        sourcemap: true
      },
    }
  },

That's right about the import. I'm not sure what's causing it not to process it the same way in the Craft project.

cavellblood commented 3 years ago

So I was wondering if something compiles differently if it has backend integration. Just a long shot. But that might be more of a question for Evan You?

khalwat commented 3 years ago

Compare your vite.config.js in the Craft project with the one generated by the Vite scaffolding.

cavellblood commented 3 years ago

The Vite scaffolding does not create a vite.config.js. And this is the vite.config.js from the Craft project which is the same as https://github.com/nystudio107/craft/blob/craft-vite/buildchain/vite.config.js:

import vue from '@vitejs/plugin-vue'
import legacy from '@vitejs/plugin-legacy'
import ViteRestart from 'vite-plugin-restart';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import critical from 'rollup-plugin-critical';
import path from 'path';

// https://vitejs.dev/config/
export default ({ command }) => ({
  base: command === 'serve' ? '' : '/dist/',
  build: {
    emptyOutDir: true,
    manifest: true,
    outDir: '../cms/web/dist',
    rollupOptions: {
      input: {
        app: '/src/js/app.ts',
      },
      output: {
        sourcemap: true
      },
    }
  },
  plugins: [
    critical({
      criticalUrl: 'https://nystudio107.com',
      criticalBase: '../cms/web/dist/criticalcss/',
      criticalPages: [
        { uri: '/', template: 'index' },
      ],
      criticalConfig: {
      }
    }),
    legacy({
      targets: ['defaults', 'not IE 11']
    }),
    nodeResolve({
      moduleDirectories: [
        path.resolve('./node_modules'),
      ],
    }),
    ViteRestart({
      reload: [
        '../src/templates/**/*',
      ],
    }),
    vue(),
    // Static Asset Fixer, see: https://github.com/vitejs/vite/issues/2394
    {
      name: 'static-asset-fixer',
      enforce: 'post',
      apply: 'serve',
      transform: (code, id) => {
        return {
          code: code.replace(/\/src\/(.*)\.(svg|jp?g|png|webp)/g, 'http://localhost:3000/src/$1.$2'),
          map: null,
        }
      },
    },
  ],
  publicDir: '../src/public',
  resolve: {
    alias: {
      '@': '/src',
    },
  },
  server: {
    host: '0.0.0.0',
  }
});
cavellblood commented 3 years ago

I'll maybe post an issue about it over here: https://github.com/vitejs/vite/issues

khalwat commented 3 years ago

You have no vite.config.js in the scaffolding at all? I'm not sure I understand? Show me exactly how you're creating this Vite app?

cavellblood commented 3 years ago

In step 1 of the Expected Behavior of the issue I posted this:

npm init vite@latest my-vanila-app --template vanilla
cd my-vanilla-app
npm i flickity

Also you could try it with npm i jquery and get the same results.

cavellblood commented 3 years ago

And then try to import jQuery from 'jquery' and console.log(jQuery).

I apologize that I edited my original post so many times. That could be a cause of a lot of the confusion here. I accidentally hit the wrong keystroke and it submitted the post before I was done with it.

khalwat commented 3 years ago

Okay, so it's using whatever the default internal config is, then, if that file doesn't exist.

cavellblood commented 3 years ago

Right.

khalwat commented 3 years ago

If you copy the vite.config.js from the Craft scaffolding to your newly created vanilla scaffolding, what happens (will need to update the package.json, etc. too?

cavellblood commented 3 years ago

Good question. Hadn't tried that yet. I'll give that a try.

whmountains commented 3 years ago

I work with @cavellblood, here are my thoughts on the issue. (Good suggestion to test, BTW.)

It seems that the Vite build does not work with any CommonJS-only NPM package. Most packages support ES Modules, but there are a few holdouts like jQuery and flickity.

You can tell if a package supports ES modules by looking at the package.json: it should have a field like module, jsnext:main, or jsnext. This is mentioned in the Vite documentation. Originally we had the issue with Flickity, but then we went looking for a more popular CommonJS-only package and landed on jQuery.

I think the default Vite config handles this correctly, but the config in this repository somehow breaks it. We have tested creating a brand-new project from the craft-vite branch and attempting to import jQuery, but it fails in the same way.

My theory is that somehow the files are not getting processed by @rollup/plugin-commonjs, which Vite uses internally. But I have no idea why that might be.

khalwat commented 3 years ago

I think the default Vite config handles this correctly, but the config in this repository somehow breaks it. We have tested creating a brand-new project from the craft-vite branch and attempting to import jQuery, but it fails in the same way

This sounds right to me!

When I encounter this, I usually look for more modern, ESM packages... there is CommonJS plugin but I think this goes against the "Vite way" to some extent.

My theory is that somehow the files are not getting processed by @rollup/plugin-commonjs, which Vite uses internally. But I have no idea why that might be.

Interesting, I thought you had to explicitly use that plugin. If that's the case, then likely something in the config or in one of the plugins is causing it.

I'd start by disabling the plugins until it works.

khalwat commented 3 years ago

I'd be curious to hear the results here, to see if it's some config setting or some plugin that is interfering in the CommonJS loader that would normally be used.

khalwat commented 3 years ago

lmk if you folks managed to figure any of this out. I may try myself as well.

cavellblood commented 3 years ago

Will do. I plan on working on it this afternoon.

cavellblood commented 3 years ago

Well I was able to get jQuery to work this afternoon but not Flickity. Kind of strange. Some things that I noticed that helped was to remove the leading slash everywhere this string appeared: /src/js/app.js, namely:

I then updated the app.js contents to this:

import $ from "jquery";

console.log($);

$("#component-container").html("It Works!");

And that worked. But I wasn't able to get Flickity working. I'll get back to this on Monday.

cavellblood commented 3 years ago

Ok so I have some results. I just cloned down a clean version of the nystudio107/craft to verify what works with minimal changes. I just had to change one character for it to work: remove the leading slash on line 17 of vite.config.js.

export default ({ command }) => ({
  base: command === 'serve' ? '' : '/dist/',
  build: {
    emptyOutDir: true,
    manifest: true,
    outDir: '../cms/web/dist',
    rollupOptions: {
      input: {
        app: 'src/js/app.ts',  // <-- This used to be `/src/js/app.ts` 
      },
      output: {
        sourcemap: true
      },
    }
  },
  ...
}

I also had to declare the modules in the shims.d.ts or else it wouldn't work.

declare module "app";
declare module "jquery";
declare module "flickity";

I'm not super familiar with TypeScript so this was probably a beginners mistake.

khalwat commented 3 years ago

Wow, that's crazy. Nice find!!

khalwat commented 3 years ago

Did you also need to change it here: https://github.com/nystudio107/craft/blob/craft-vite/cms/templates/_boilerplate/_partials/critical-css.twig#L12

{{ craft.vite.script("/src/js/app.ts", false) }}

?

cavellblood commented 3 years ago

I tried changing it there and it didn't seem to have any effect. I thought that maybe your craft-vite plugin took all that into account.

cavellblood commented 3 years ago

It worked with the leading slash.

cavellblood commented 3 years ago

I should note that I did all these tests when make dev was running. I didn't but should've run these tests after doing a make build to verify. I can try that now but I'm guessing it would work.

khalwat commented 3 years ago

yep might as well make build and give it a whirl that way too

cavellblood commented 3 years ago

Also, just curious about the best way to test built files. Here's what I currently do:

Is that the best way to test built files?

khalwat commented 3 years ago

You can do it that way, or you can stop the vite container with:

docker-compose stop vite

but I typically just leave it running and do

make build

as you've suggested

khalwat commented 3 years ago

That's one of the selling points of Docker-izing things, you can spin up as many instances as you need/want

cavellblood commented 3 years ago

Ok.

What's really odd now is that when I clone down cavellblood/craft Vite doesn't seem to be working at all. It says it's running in the terminal but on the front end all I see is this:

Screen Shot 2021-08-23 at 4 51 32 PM

Maybe there's something awry with my machine. I'll try cloning down your repo to see if I have the same issue.

cavellblood commented 3 years ago

It's not loading the app.ts file anymore.

cavellblood commented 3 years ago

Same thing is happening when I clone your repo down. Time to restart some things.

cavellblood commented 3 years ago

I'm not sure what happened. I've cloned your repo down twice and I can't get the front end to load. Everything was working just fine a moment ago. I'll have to try a different computer. Sorry that this verification is taking longer than expected. I've even removed old images from Docker after quitting the project.

Anyway, I'll get back to you when I get it working.

khalwat commented 3 years ago

I made you change to the Spoke and Gear P&T demo site that I converted over to Vite, and it's working as expected:

https://github.com/nystudio107/spoke-and-chain/commit/b9a896c18555a85b1cde441b6ac1dc0f1baa2afd

(but it has always worked -- just noting that nothing broke)

cavellblood commented 3 years ago

Right. Well I just was able to get things working again. I had to adjust the path in config/vite.php and criticalcss.twig to ../src/js/app.ts. I'm adding it to the PR.

khalwat commented 3 years ago

Looks like the right way to do this is just change it to this in buildchain/vite.config.js:

      input: {
        app: './src/js/app.ts',
      },

...then you don't need to change anything anywhere else, at least in my testing.

cavellblood commented 3 years ago

Ok. Good to know. I should've tried that. I'll have to test that later but I think we can close this issue now.

khalwat commented 3 years ago

I'm going to do a little more testing but you've been fantastic, thank you for digging into this!

khalwat commented 3 years ago

I was able to get Europa Museum ported over to Vite, thanks to this finding!

https://github.com/nystudio107/europa-museum/tree/docker-vite

cavellblood commented 3 years ago

That's great!

So did you eventually just settle on this change:

input: {
    app: './src/js/app.ts',
},

I guess I could take a look at that repo to see what you did.

cavellblood commented 3 years ago

I see the changes you made here: https://github.com/nystudio107/craft/commit/e213d4f3ce8497c875c07b5480f45cf405f48790

khalwat commented 3 years ago

You got it!