Alecaddd / awps

A Modern WordPress Starter Theme for savvy Developers
GNU General Public License v3.0
413 stars 108 forks source link

Import Order in ES6 / Dependencies #4

Closed SamThilmany closed 7 years ago

SamThilmany commented 7 years ago

I noticed this bug for the first time in the awps theme, but it was also very simple to reproduce it in a non WordPress-based project. If the import-function in ES6 is used like this,

import $ from 'jquery';
    window.$ = $;
    window.jQuery = $;
import 'izimodal';

one would expect, that all the imports other than jQuery, which may be dependent on the latter, are loaded correctly, since jQuery has already been defined before. Unfortunately, JS is not executed consecutively and so very often an error message appears in the console, which tell one, that the script XY needs jQuery:

iziModal requires jQuery

Probably it only works, if script A, that is dependent of script B, is bigger than script B, because then script A will be finished earlier with processing and thus be in front of script B in the resulting main.min.js. In my case, the code bit throwing the error was in line 8 of my main.min.js, so definitely before the jQuery code.

Here is the code bit of iziModal:

if (typeof jQuery === "undefined") {
     throw new Error("iziModal requires jQuery");
}

Does anyone have an idea how to handle this bug?

PS: In Alex' video, this bug is not noticeable, because WordPress integrates jQuery before loading other scripts and Alex did not unregister WP jQuery.

Alecaddd commented 7 years ago

Oopsie, what a silly developer I am ๐Ÿ˜ฅ

I totally forgot about testing the jQuery import, and now that I see it properly, in ES6 the window variables are not accepted anymore, that's why other packages can't read it.

I need to update the gulp workflow to properly compile and package everything.

I'll fix it ASAP and push the updated version. Thanks for flagging this out.

Alecaddd commented 7 years ago

@SamThilmany Ok, it should be solved now.

What I had to do was implementing browserify-shim in order to properly import variables to be used globally.

If you pull the latest code you will notice that I'm using require instead of import for jquery, and declaring the $ variable

/**
 * Manage global libraries like jQuery or THREE from the package.json file
 */
var $ = require( 'jquery' );

For globally required variables, if you want to use my method in order to package and bundle all the scripts into a single one, you need to update the package.json file.

Here's the example related to jquery

"browser": {
        "jquery": "./node_modules/jquery/dist/jquery.js"
    },
    "browserify-shim": {
        "jquery": "$"
    },

Here I'm declaring the global variable $ to use jquery from the node_modules folder. I'm declaring the "jquery" alias in the "browser" declaration so I can use the require('jquery') without redeclaring again the full path of the file.

I also deregistered by default the built-in jquery version of WordPress. Let me know if it works for you.

SamThilmany commented 7 years ago

@Alecaddd Thank you very much for your quick changes. Unfortunately it seems, that this does not do the trick (in my case at least).

The packages still yell at me, because they require jQuery. I tried with browserify-shim and browserify-global-shim while using your code or the one provided by the documentation of the package. In both cases 'jQuery' seems to be loaded too late...

If I manage to get it up and running, I'll definitely share it, but I don't mind if you keep you eyes open too. ๐Ÿ˜‰

Alecaddd commented 7 years ago

@SamThilmany Mhhhh, are you sure you pulled the updated code?

I left iziModal required in the main.js file and it doesn't trigger any error for me. Both jQuery and $ sign works inside imported libraries.

Also, did you run npm install again to get the new required packages?

SamThilmany commented 7 years ago

Yeah, I did run npm install and I also think, that I updated all my files. I didn't pull though, because I'm currently using your gulp-code in a non-WordPress project.

I now added

"browserify-shim": {
    "jquery": "$",
    "main": {
        "depends": [
            "jquery"
        ]
    }
},

which seems to do the trick with jQuery, but now he tells me that he can't find the module:

Error: Cannot find module 'tether'

If I comment out tether, It's the next module which is included in main.min.css which cannot be found. This is really weird to me...

If you like to see the code, I just made a git repo. Note that I didn't use your latest version of the gulpfile, because I don't have an account.js and so I also don't need to map over several files.

SamThilmany commented 7 years ago

I just set up a WordPress site for testing your latest awps theme and it works well, even if I don't understand why. But, it only works using the iziModal package (and maybe some others, who knows). I now tried to include Bootstrap and guess what, there's a little issue here.

ReferenceError: Can't find variable: jQuery

Alecaddd commented 7 years ago

Thanks for the extra info, I'm glad it works on a standard WP installation (that was my main goal ๐Ÿ˜›), but it doesn't make sense not working externally, or with other libraries like Bootstrap. I will investigate more and let you know!

Alecaddd commented 7 years ago

@SamThilmany Any updates on this issue?

If it works with WordPress and the theme, I would like to close it.

SamThilmany commented 7 years ago

No, it still does not work. I just set up a new local site with your latest code by using your awps-cli. When adding import 'bootstrap' I still get the error message:

Error: Bootstrap's JavaScript requires jQuery. jQuery must be included before Bootstrap's JavaScript.

PS: The main.js now looks like this:

/**
 * Manage global libraries like jQuery or THREE from the package.json file
 */
var $ = require( 'jquery' );

// Import libraries
import 'bootstrap';
import 'tether';
import 'slick-carousel';
import 'izimodal';

// Import custom modules
import App from'./modules/app.js';
import Carousel from './modules/carousel.js';

const app = new App();
const carousel = new Carousel();
Alecaddd commented 7 years ago

I tested it and it works for me!

Here's what I did:

  1. Installed Bootstrap via npm npm install bootstrap@3
  2. Imported the compiled and minified CSS into my style.scss file
    @import '../../node_modules/bootstrap/dist/css/bootstrap.min.css';
  3. Imported Bootstrap in my main.js file
    import 'bootstrap';

Compiled everything with Gulp with the latest version of AWPS, the one with this configuration of browserify-shim inside package.json

"browserify": {
        "transform": [
            "browserify-shim"
        ]
    },
    "browser": {
        "jquery": "./node_modules/jquery/dist/jquery.js"
    },
    "browserify-shim": {
        "jquery": "$"
    }

Everything works for me, including tooltips, modals, carousel, etc.

SamThilmany commented 7 years ago

Hmmm... I've exactly the same code ๐Ÿ˜•

Tested in Safari 10.1.1 (12603.2.4) and Chrome 58.0.3029.110 (64-bit).

The error message is Chrome is a bit longer:

Uncaught Error: Bootstrap's JavaScript requires jQuery. jQuery must be included before Bootstrap's JavaScript. at Object.n.1 (main.min.js:1) at o (main.min.js:1) at main.min.js:1 at Object.n.6../modules/app.js (main.min.js:7) at o (main.min.js:1) at t (main.min.js:1) at main.min.js:1

Alecaddd commented 7 years ago

Are you sure you have my same package.json file, with my same exact configuration? If it's different, please use mine, delete your node_modules folder and run npm install again.

You also said that you're using my code outside a WordPress theme. How did you update the gulpfile.js did something change there? Be sure the JS compiling follows my same order of browserify and babelify.