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

Loading FancyTree widget destroys all previous jQueryUI instances #803

Closed
creage opened this issue Nov 14, 2017 · 5 comments
Closed

Loading FancyTree widget destroys all previous jQueryUI instances #803

creage opened this issue Nov 14, 2017 · 5 comments

Comments

@creage
Copy link

creage commented Nov 14, 2017

FancyTree includes custom jQueryUI, and loading FancyTree widget destroys all of previous instances of jQueryUI.

Particularly $.effects, $.widget, $.Widget get completely overwritten, destroying all of custom extensions applied before FancyTree is loaded.

Environment

  • Browser type and version: Chrome 62
  • jQuery and jQuery UI versions: "jquery": "3.2.1", "jquery-ui": "1.12.1",
  • Fancytree version: "jquery.fancytree": "2.26.0"
    enabled/affected extensions: jQueryUI
@mar10
Copy link
Owner

mar10 commented Nov 14, 2017

How did you include Fancytree?

@creage
Copy link
Author

creage commented Nov 15, 2017

We use Webpack to manage async loading of modules, on demand. We first load our jQueryUI library on application start, and then, when we come to FancyTree usage, we load it separately. Since jQuery is defined globally, loading FancyTree affects this single instance of it.

Currently we workaround that issue by just replacing FancyTree's jquery.fancytree.ui-deps dependency with our instance of jQueryUI, as it already has everything FancyTree needs. But still, it would be nice if jquery.fancytree.ui-deps merges to existing definitions, and not just overwrite them blindly.

Here is s list of problem code:
1
2
3
4
5
And this is correct way of merging.

@mar10
Copy link
Owner

mar10 commented Nov 15, 2017

Thanks for reporting this. The module support is new with 2.25 and there are still use cases that need to be fixed.

jquery.fancytree.ui-deps.js is in fact the original jQuery UI custom build.

Does one of this work for you:

  1. Add another alias
    We use an alias in webpack.config to make sure the same global $ instance is used. (see here).
    Maybe it is possible to add another alias for your case?
    Something like this, if that makes any sense:
resolve: {
    ...
    alias: {
      // NOTE: Required to load jQuery Plugins into the *global* jQuery instance:
      'jquery': require.resolve('jquery'),
      'jquery.fancytree.ui-deps': require.resolve('jquery-ui')
    }
  },
  1. Simply return if $.ui exists

Maybe it's even simpler if that works, by modifying jquery.fancytree.ui-deps.js:

...
}(function( $ ) {

if( $.ui ) {
    return;
}
$.ui = $.ui || {};

@creage
Copy link
Author

creage commented Nov 16, 2017

Well, first solution would definitely work, but why would we load two instances of jQueryUI, if we already have everything FancyTree needs? This will increase the assets size with no reason, as I see it.

Second solution will break your code, as you can not be sure, that existing jQueryUI has everything FancyTree needs, right?

So, ideally, every definition in jquery.fancytree.ui-deps.js should first check, if such item already exists, and define itself only if it does not.

P.S. In order someone else face with such issue, here is our workaround

// replace custom jQueryUI build for FancyTree
		new webpack.NormalModuleReplacementPlugin(/jquery\.fancytree\.ui-deps/, function (resource) {

			if (resource.userRequest) {

				const map = {
					find: /client\\node_modules\\jquery\.fancytree\\dist\\modules\\jquery\.fancytree\.ui-deps/,
					replace: 'client\\sources\\App\\vendor\\jquery\.fancytree\.ui-deps'
				};

				resource.request = resource.request.replace(map.find, map.replace);
				resource.userRequest = resource.userRequest.replace(map.find, map.replace);
				resource.resource = resource.resource.replace(map.find, map.replace);
			}
		})

and our custom jquery.fancytree.ui-deps.js has just single import

// custom jQueryUI replacement, as we already have everything FancyTree needs, except blind-effect
import 'jquery-ui/ui/effects/effect-blind';

@mar10
Copy link
Owner

mar10 commented Nov 16, 2017

but why would we load two instances of jQueryUI, if we already have everything FancyTree needs? This will increase the assets size with no reason, as I see it.

I was thinking that the alias simply assures that jquery-ui is used instead of jquery.fancytree.ui-deps.js. That's how it worked with the jquery alias at least.
Havn't tried it though, have you?

Second solution will break your code, as you can not be sure, that existing jQueryUI has everything FancyTree needs, right?

true

So, ideally, every definition in jquery.fancytree.ui-deps.js should first check, if such item already exists, and define itself only if it does not.

I was trying to avoid to patching the original source, since it would be error prone on updates.
But given that jQuery UI development is rather lame recently that might not be a problem anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants