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

ES6 module doesn't work without 'gridstack/dist/jquery' - 2 versions of jquery bundled as a result #1306

Closed
gregid opened this issue Jul 25, 2020 · 17 comments · Fixed by #1714
Labels

Comments

@gregid
Copy link
Contributor

gregid commented Jul 25, 2020

Subject of the issue

When importing as ES6 module gridstack doesn't recognize existing jquery import. You must import gridstack/dist/jquery for gridstack to work. This results in unnecessary 2 versions of jquery being bundled.

Your environment

  • version of gridstack.js: 1.1.2
  • any browser
  • jquery 3.4.1, 3.5.1 tested

Steps to reproduce

Uncomment any commented line and comment out import 'gridstack/dist/jquery'

// import $ from 'jquery';
// import { jQuery } from 'jquery';
// import 'jquery';

import 'gridstack/dist/jquery'; // doesn't work without this line

import 'gridstack/dist/gridstack';
import 'gridstack/dist/gridstack.jQueryUI';
import 'gridstack/dist/gridstack-poly.min.js';
import 'gridstack/dist/jquery-ui';
import 'gridstack/dist/gridstack.css';

Expected behaviour

Importing standard jquery should be sufficient for gridstack to work.

Actual behaviour

Instead it results in the following error

gridstack.jQueryUI.js:16 Uncaught ReferenceError: jQuery is not defined
    at gridstack.jQueryUI.js:16
    at Object.15df57f963fba245a3dd1dd99844b498 (gridstack.jQueryUI.js:18)
    at newRequire (app2.a577e0b1.js:68)
    at localRequire (app2.a577e0b1.js:80)
    at Object.dc637c3e3d40dc66c1f97cf67d9d6d1f.underscore (WidgetGridView.js:13)
    at newRequire (app2.a577e0b1.js:68)
    at localRequire (app2.a577e0b1.js:80)
    at Object.e5e83521c6a34a6c4a7cb873b81ab307../V01 (V01.index.js:15)
    at newRequire (app2.a577e0b1.js:68)
    at localRequire (app2.a577e0b1.js:80)
@adumesny
Copy link
Member

first off jquery-ui should be placed before gridstack code - it''s very possible the custom generated version isn't importing exported JQ correctly and might need to be fixed. Can you try that ?

For now you can import the entire normal JQUI instead of our custom version (which is 1/5th the size if you don't use any of the UI components)

@gregid
Copy link
Contributor Author

gregid commented Jul 25, 2020

Thanks @adumesny I have tried so many variations - including importing jquery-ui/ jquery-ui-bundle/jquery-ui-dist but it just seems jquery-ui won't work with ES6. It looks like a losing battle.

@adumesny
Copy link
Member

can you post a reproducible test ? then I can mod jQueryUI.js until it runs...

@gregid
Copy link
Contributor Author

gregid commented Jul 25, 2020

Will do

@gregid
Copy link
Contributor Author

gregid commented Jul 25, 2020

@adumesny here is a sample app I made:
https://github.com/gregid/gridstack-parcel-app

@adumesny
Copy link
Member

hey thanks for the sample!
if you try just these you get the error as well, so JQ UI 1.12.1 (which is no longer maintained) is NOT compatible with ES6 ?

import 'jquery';
import 'jquery-ui';

this is probably a general problem hopefully with a fix out there we can re-use, since we have our own copy. Interestingly our version of JQ + JQUI does work (I recall commenting some code that prevented export from happening - will have to ding when I have time).

@gregid
Copy link
Contributor Author

gregid commented Jul 26, 2020

@adumesny that's my impression as well that jquery-ui simply doesn't want to work with ES6. I think there is no point in investing time in making gridstack 1.x fix this issue and just make sure that 2.x will work with ES6 as default and with named exports i.e.:

import GridStack from 'gridstack';
// or
import { GridStack } from 'gridstack';

rather than import 'gridstack'
I tried to see if I could make typescript version with rollup and had a little success with https://github.com/wessberg/rollup-plugin-ts but I don't know enough typescript to finalize it.

@adumesny
Copy link
Member

adumesny commented Jul 26, 2020

you mean you played with the typescript 2.x branch ? yeah, that's what I've been focusing on (and new fixes) but we still have to make JQ + JQUI work as drag&drop still uses it for now (but all JQ code has been removed from main classes).

Note that I had to make this change (not sure why to JQ 3.4.1 and likely 3.5.1 has same issue ?

// Expose jQuery and $ identifiers, even in AMD
// (#7102#comment:10, https://github.com/jquery/jquery/pull/557)
// and CommonJS for browser emulators (#13566)
//if ( !noGlobal ) { [alain] noGlobal=true when using webpack/TS, yet we compile this in, not AMD load, so always expose
	window.jQuery = window.$ = jQuery;
//}

and in jq UI 1.12.1

(function( factory ) {
  /* [alain] we compile this in so no need to load with AMD
  if ( typeof define === "function" && define.amd ) {

    // AMD. Register as an anonymous module.
    define([ "jquery" ], factory );
  } else */{

    // Browser globals
    factory( jQuery );
  }
}(function( $ ) {
...

@gregid
Copy link
Contributor Author

gregid commented Jul 27, 2020

In that case it must be done anyway. Since (at least in typescript branch) you are using webpack there are some workarounds like:
https://www.codeproject.com/Tips/5273499/Tip-about-Importing-Old-Fashioned-JavaScript-Libra
But still hacks only and not a proper solutions.

@adumesny
Copy link
Member

adumesny commented Aug 1, 2020

closing this since as it's not a gridstack lib issue, but jquery not exporting jQuery with ES6 import for jqueryui to work.

@adumesny adumesny closed this as completed Aug 1, 2020
@adumesny adumesny reopened this Aug 10, 2020
@adumesny
Copy link
Member

adumesny commented Aug 10, 2020

got it working in 2.x where I'm able to do this and all works great (though you can't import your own jquery, but we do export $ now). Will push 2.x out hopefully soon...

import { GridStack, $ } from 'gridstack';
import 'gridstack/dist/gridstack.css';

@adumesny
Copy link
Member

closing this as solved in 2.0.0-rc and later builds

@adumesny
Copy link
Member

adumesny commented Sep 4, 2020

FYI this also works in just released 1.2.1 with this:

import 'gridstack/dist/jquery';
import 'gridstack/dist/jquery-ui';

import 'gridstack/dist/gridstack-poly'; // optional IE support
import { GridStack } from 'gridstack';
import 'gridstack/dist/gridstack.jQueryUI';
import 'gridstack/dist/gridstack.css';

2.x is still better (other fixes) and simpler import...

@gondias
Copy link

gondias commented Nov 23, 2020

Hi,
First of all, thank you for your hard work on gridstack. I love it. While upgrading my jquery mvc web app to use gridstack version 2 i started having issues with other plugins not being defined. I noticed some comments on npm and i then found this ticket. Is there anything i can do to use V2 and still have everything else working?
Thanks for the help.
Kind regards,
GD

@adumesny
Copy link
Member

@gondias no time to focus on jquery - actually spending time removing it! you can't bring your own jquery with 2.x, so I will re-open this.

But very soon 3.0 you will be able to use the html5 native drag&drop so your code could use your version of jquery. I don't know how to package this legacy jq code and make it work for everyone. Don't forget to contribute or donate. thanks.

@adumesny adumesny reopened this Nov 23, 2020
@gondias
Copy link

gondias commented Nov 23, 2020

@adumesny Thanks for the quick reply!

@adumesny
Copy link
Member

adumesny commented Dec 5, 2020

closing as v3 html5 version is a better workaround (no jquery).

@adumesny adumesny closed this as completed Dec 5, 2020
adumesny added a commit to adumesny/gridstack.js that referenced this issue Apr 6, 2021
* fix gridstack#1709
also fix gridstack#1306 I believe.
* correct info in Readme for using JQ version and ES6 (tested in Angular app)
* changed .ts to include jquery by name instead of location.
@adumesny adumesny mentioned this issue Apr 6, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants